Omniscia Powercity Audit
CollSurplusPool Static Analysis Findings
CollSurplusPool Static Analysis Findings
CSP-01S: Inexistent Event Emission
Type | Severity | Location |
---|---|---|
Language Specific | CollSurplusPool.sol:L60 |
Description:
The linked function adjusts a sensitive contract variable yet does not emit an event for it.
Example:
43function setAddresses(44 address _borrowerOperationsAddress,45 address _troveManagerAddress,46 address _activePoolAddress,47 address _pulseX48)49 external50 override51 onlyOwner52{53 checkContract(_borrowerOperationsAddress);54 checkContract(_troveManagerAddress);55 checkContract(_activePoolAddress);56
57 borrowerOperationsAddress = _borrowerOperationsAddress;58 troveManagerAddress = _troveManagerAddress;59 activePoolAddress = _activePoolAddress;60 pulseX = IERC20(_pulseX);61
62 emit BorrowerOperationsAddressChanged(_borrowerOperationsAddress);63 emit TroveManagerAddressChanged(_troveManagerAddress);64 emit ActivePoolAddressChanged(_activePoolAddress);65
66 _renounceOwnership();67}
Recommendation:
We advise an event
to be declared and correspondingly emitted to ensure off-chain processes can properly react to this system adjustment.
Alleviation (8bedd3b0df6387957e6b8f5d52507e776c1458b0):
The PulseXAddressChanged
event has been introduced to the codebase and is now emitted whenever the pulseX
is set, addressing this exhibit's concerns.
CSP-02S: Inexistent Sanitization of Input Address
Type | Severity | Location |
---|---|---|
Input Sanitization | CollSurplusPool.sol:L47 |
Description:
The linked function accepts an address
argument yet does not properly sanitize it.
Impact:
The presence of zero-value addresses, especially in constructor
implementations, can cause the contract to be permanently inoperable. These checks are advised as zero-value inputs are a common side-effect of off-chain software related bugs.
Example:
43function setAddresses(44 address _borrowerOperationsAddress,45 address _troveManagerAddress,46 address _activePoolAddress,47 address _pulseX48)49 external50 override51 onlyOwner52{53 checkContract(_borrowerOperationsAddress);54 checkContract(_troveManagerAddress);55 checkContract(_activePoolAddress);56
57 borrowerOperationsAddress = _borrowerOperationsAddress;58 troveManagerAddress = _troveManagerAddress;59 activePoolAddress = _activePoolAddress;60 pulseX = IERC20(_pulseX);61
62 emit BorrowerOperationsAddressChanged(_borrowerOperationsAddress);63 emit TroveManagerAddressChanged(_troveManagerAddress);64 emit ActivePoolAddressChanged(_activePoolAddress);65
66 _renounceOwnership();67}
Recommendation:
We advise some basic sanitization to be put in place by ensuring that the address
specified is non-zero.
Alleviation (8bedd3b0df6387957e6b8f5d52507e776c1458b0):
The _pulseX
address is now properly sanitized in the CollSurplusPool::setAddresses
function by passing it in the CheckContract::checkContract
modifier, alleviating this exhibit.
CSP-03S: Improper Invocations of EIP-20 transfer
/ transferFrom
Type | Severity | Location |
---|---|---|
Standard Conformity | CollSurplusPool.sol:L101, L129 |
Description:
The linked statements do not properly validate the returned bool
values of the EIP-20 standard transfer
& transferFrom
functions. As the standard dictates, callers must not assume that false
is never returned.
Impact:
If the code mandates that the returned bool
is true
, this will cause incompatibility with tokens such as USDT / Tether as no such bool
is returned to be evaluated causing the check to fail at all times. On the other hand, if the token utilized can return a false
value under certain conditions but the code does not validate it, the contract itself can be compromised as having received / sent funds that it never did.
Example:
101bool success = pulseX.transfer( _account, claimableColl);
Recommendation:
Since not all standardized tokens are EIP-20 compliant (such as Tether / USDT), we advise a safe wrapper library to be utilized instead such as SafeERC20
by OpenZeppelin to opportunistically validate the returned bool
only if it exists in each instance.
Alleviation (8bedd3b0df6387957e6b8f5d52507e776c1458b0):
The Powercity team has stated that they intend to utilize the PulseX token solely and that it reverts on unsuccessful transfers, rendering an evaluation of the yielded bool
redundant.
As such, we consider this exhibit nullified based on the fact that a known failure-reverting EIP-20 asset will be utilized in the referenced invocations.