Canto contest - 0xf15ers's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Comptroller.sol#L1469-L1472

Vulnerability details

Impact

The hardcoded return value is the address of COMP token in mainnet not of WETH in getWETHAddress().

• https://etherscan.io/address/0xc00e94Cb662C3520282E6f5717214004A7f26888

• grantCompInternal() function depends on this return value which will cause unintended issues.

Proof of Concept

 function grantCompInternal(address user, uint amount) internal returns (uint) {
        WETH comp = WETH(payable(getWETHAddress())); // called here
        uint compRemaining = comp.balanceOf(address(this));
        if (amount > 0 && amount <= compRemaining) {
            comp.transfer(user, amount);
            return 0;
        }
        return amount;
    }


function getWETHAddress() virtual public view returns (address) {
        return 0xc00e94Cb662C3520282E6f5717214004A7f26888;
    }

Tools Used

• Manual analyis and Etherscan

Judge has assessed an item in Issue #210 as High risk. The relevant finding follows:

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/CNote.sol#L96-L129

Vulnerability details

Impact

• repayAmountFinal is not used inside repayBorrowFresh()
• The computation for actualRepayAmount doesnot account for repayAmountFinal in L111.

Proof of Concept

function repayBorrowFresh(address payer, address borrower, uint repayAmount) internal override returns(uint){
       .................
        uint repayAmountFinal = repayAmount == type(uint).max ? accountBorrowsPrev : repayAmount;

        //this cannot be a require statement, it must be accounted for if this is the case
        require(getCashPrior() == 0, "CNote::repayBorrowFresh:Liquidity in Note Lending Market is always flashed");
        //make sure that this is remedied
	
        ...................
        uint actualRepayAmount = doTransferIn(payer, repayAmount);
        ..........
        }

Tools Used

Manual analysis

Recommended Mitigation Steps

Use uint actualRepayAmount = doTransferIn(payer, repayAmountFinal);

1. Unused local variables, function parameters

• In getBorrowRate, the paramters uint cash, uint borrows, uint reserves are not used inside the function.
• in NoteInterest.sol#L109, uint reserveFactorMantissa parameter is unsused.

2. Unused modifier

• The modifer OnlyUniGov is not used anywher inside the contract Proposal-Store.sol

3. Misleading/Random comments

• https://github.com/Plex-Engineer/manifest/blob/688e9b4e7835854c22ef44b045d6d226b784b4b8/contracts/Proposal-Store.sol#L6-L7
• The comments may be misleading in Proposal-Store.sol

4. function visibility can be restricted to pure and view

• function redeemVerify()'s visibility can be changed to pure
• function liquidateBorrowAllowed() visibility can be changed to view

5. Add proper revert string in require()

• https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/Treasury/TreasuryDelegate.sol#L16-L17

6. No events emitted for important updates

• https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/WETH.sol#L32
• https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/WETH.sol#L24
• The event emits are commented but it's a good practice to emit event for important actions like deposit() and withdraw()

7. allowance(address owner, address spender) in WETH.sol returns 0 always

• https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/WETH.sol#L103-L104
• It's missing keyword return

// recommendation
function allowance(address owner, address spender) external view returns (uint256) {
	    return _allowance[owner][spender];
    }

1. Default vaule initialisation can be omitted

• For eg: address(0) in Note.sol#L6

2. Use revert string < 32 bytes to save gas

• https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Note.sol#L27
• https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/NoteInterest.sol#L168
• https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/NoteInterest.sol#L154

3. Cache array length in loops instead of computing length in every iteration

for eg.

uint256 len = assets.length;
for(uint256 i = 0; i < len; i++) {
  ...
}

> grep -rn '.' -e 'for.*[.]length'
./Comptroller.sol:736:        for (uint i = 0; i < assets.length; i++) {
./Comptroller.sol:960:        for (uint i = 0; i < allMarkets.length; i ++) {
./Comptroller.sol:1107:        for (uint i = 0; i < affectedUsers.length; ++i) {
./Comptroller.sol:1348:        for (uint i = 0; i < cTokens.length; i++) {
./Comptroller.sol:1354:                for (uint j = 0; j < holders.length; j++) {
./Comptroller.sol:1360:                for (uint j = 0; j < holders.length; j++) {
./Comptroller.sol:1365:        for (uint j = 0; j < holders.length; j++) {
./ComptrollerG7.sol:719:        for (uint i = 0; i < assets.length; i++) {
./ComptrollerG7.sol:942:        for (uint i = 0; i < allMarkets.length; i ++) {
./ComptrollerG7.sol:1233:        for (uint i = 0; i < cTokens.length; i++) {
./ComptrollerG7.sol:1239:                for (uint j = 0; j < holders.length; j++) {
./ComptrollerG7.sol:1246:                for (uint j = 0; j < holders.length; j++) {
./Lens/CompoundLens.sol:343:        for (uint i = 0; i < proposalIds.length; i++) {
./Lens/CompoundLens.sol:404:        for (uint i = 0; i < proposalIds.length; i++) {

4. Prefix increment (++i) is cheaper than postfix increment (i++)

Using prefix increment saves small amount of gas than postfix increment because it returns the updated value hence doesn't requires to store intermediate value. This can be more significant in loops where this operation is done multiple times.

for eg.

// before
for(uint256 i = 0; i < len; i++) {
  ...
}

// Replace with
for(uint256 i = 0; i < len; ++i) {
  ...
}

./Comptroller.sol:206:        for (uint i = 0; i < len; i++) {
./Comptroller.sol:736:        for (uint i = 0; i < assets.length; i++) {
./Comptroller.sol:1006:        for(uint i = 0; i < numMarkets; i++) {
./Comptroller.sol:1348:        for (uint i = 0; i < cTokens.length; i++) {
./ComptrollerG7.sol:116:        for (uint i = 0; i < len; i++) {
./ComptrollerG7.sol:196:        for (uint i = 0; i < len; i++) {
./ComptrollerG7.sol:719:        for (uint i = 0; i < assets.length; i++) {
./ComptrollerG7.sol:963:        for(uint i = 0; i < numMarkets; i++) {
./ComptrollerG7.sol:1233:        for (uint i = 0; i < cTokens.length; i++) {
./Lens/CompoundLens.sol:166:        for (uint i = 0; i < cTokenCount; i++) {
./Lens/CompoundLens.sol:211:        for (uint i = 0; i < cTokenCount; i++) {
./Lens/CompoundLens.sol:235:        for (uint i = 0; i < cTokenCount; i++) {
./Lens/CompoundLens.sol:268:        for (uint i = 0; i < proposalCount; i++) {
./Lens/CompoundLens.sol:290:        for (uint i = 0; i < proposalCount; i++) {
...

5. Use solidity custom errors to save gas

solidity 0.8.4 introduces custom errors which are cheaper than using revert strings in terms of gas. This can be used to save gas.

for eg.

  // Before
  require(condition, "Revert strings");

  // After
  error CustomError();
  if (!condition) {
    revert CustomError();
  }

more details can be found here