Based Loans contest - 0xRajeev's results

Handle

0xRajeev

Vulnerability details

Impact

Six of the seven Comptroller verify functions do nothing. Not sure why their calls in CToken.sol have been uncommented from the original Compound version.

Except redeemVerify(), six other verify functions transferVerify(), mintVerify(), borrowVerify(), repayBorrowVerify(), liquidateBorrowVerify() and seizeVerify() have no logic except accessing state variables to not be marked pure. Calls to these functions were commented out in the original Compound code’s CToken.sol but have been uncommented here.

Given that they do not implement any logic, the protocol should not be making any assumptions about any defence provided from their unimplemented verification logic.

Proof of Concept

Dummy functions whose comments say “// Shh - currently unused”:

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L263-L281

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L402-L418

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L450-L474

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L519-L546

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L584-L609

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L638-L656

Uncommented calls from modified code:

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CToken.sol#L126

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CToken.sol#L560

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CToken.sol#L798

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CToken.sol#L915

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CToken.sol#L1019

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CToken.sol#L1090

Commented calls from original Compound code:

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CToken.sol#L123-L124

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CToken.sol#L558-L559

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CToken.sol#L797-L798

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CToken.sol#L915-L916

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CToken.sol#L1020-L1021

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CToken.sol#L1092-L1093

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add logic to implement verification if that is indeed assumed to be implemented but is actually not. Otherwise, comment call sites.

Handle

0xRajeev

Vulnerability details

Impact

The sweepToken() function in the original Compound code whose specified purpose was to recover accidentally sent ERC20 tokens to contract has been removed.

The original code comment says: “A public function to sweep accidental ERC-20 transfers to this contract. Tokens are sent to admin (timelock).” This safety measure is helpful given the number/value of accidentally stuck tokens that are sent to contracts by mistake.

Tokens accidentally sent to this contract will be stuck leading to fund loss for sender.

Proof of Concept

https://github.com/compound-finance/compound-protocol/blob/b9b14038612d846b83f8a009a82c38974ff2dcfe/contracts/CErc20.sol#L112-L120

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CErc20.sol#L109-L121

Tools Used

Manual Analysis

Recommended Mitigation Steps

Retain this function unless there is a specific reason to remove it here.

Handle

0xRajeev

Vulnerability details

Impact

Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.6.10, ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs.

For reference, see https://swcregistry.io/docs/SWC-103

Proof of Concept

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/UniswapOracle/UniswapAnchoredView.sol#L3

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/UniswapOracle/UniswapConfig.sol#L3

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/UniswapOracle/UniswapLib.sol#L3

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove ^ in “pragma solidity ^0.6.10” and change it to “pragma solidity 0.6.12” to be consistent with the rest of the contracts.

Handle

0xRajeev

Vulnerability details

Impact

Function _setCompAddress() is used by admin to change the COMP token address. However, there is no zero-address validation on the parameter. This may accidentally set COMP token address to zero-address but it can be reset by the admin. Any interim transactions might hit exceptional behavior.

Proof of Concept

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L1350-L1357

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add zero-address check to _comp parameter of _setCompAddress().

Handle

0xRajeev

Vulnerability details

Impact

A zero or some minimum threshold check is missing for newMaxAssets parameter of _setMaxAssets() function which is used by admin to set the maximum number of assets that controls how many markets can be entered.

If accidentally set to 0 then all users cannot enter any market which will significantly affect protocol operations.

Proof of Concept

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/Comptroller.sol#L954-L971

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add zero/threshold check to newMaxAssets parameter.

Handle

0xRajeev

Vulnerability details

Impact

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling.

The doTransferOut() function was changed from using a transfer() function (which reverts) to a call() function (which returns a boolean), however there is no account existence check for the destination address to. If it doesn’t exist, for some reason, call will still return true (not throw an exception) and successfully pass the return value check on the next line.

The checked call paths don’t seem vulnerable because they use msg.sender/admin and not a user-controlled address, but this may be a risk if used later in other contexts. Hence rating as low-risk.

For reference, see this related high-risk severity finding from Trail of Bit’s audit of Hermez Network: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf

Proof of Concept

https://github.com/code-423n4/2021-04-basedloans/blob/5c8bb51a3fdc334ea0a68fd069be092123212020/code/contracts/CEther.sol#L145-L148

https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-calls

https://docs.soliditylang.org/en/v0.8.4/control-structures.html#error-handling-assert-require-revert-and-exceptions

Tools Used

Manual Analysis

Recommended Mitigation Steps

Check for account-existence before the call() to make this safely extendable to user-controlled address contexts in future.