Canto contest - hansfriese's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/WETH.sol#L47

Vulnerability details

Impact

WETH.totalSupply() returns wrong result. I can't find other contracts that use this function but WETH.sol is a base contract and it should be fixed properly.

Proof of Concept

WETH._balanceOf just returns a balance of a specific address and totalSupply must be the same as the total balance that was deposited to this contract.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

L47 should be changed like below.

return address(this).balance;

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/WETH.sol#L104

Vulnerability details

Impact

WETH.allowance() returns wrong result. I can't find other contracts that use this function but WETH.sol is a base contract and it should be fixed properly.

Proof of Concept

In this function, the "return" keyword is missing and it will always output 0 in this case.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

L104 should be changed like below.

return _allowance[owner][spender];

Summary

I've found some comments are wrong and I recommend to change comments properly when reuse standard codes from example contracts.

Low-Risk Issues (Wrong comments)

• AccountantDelegator.supplyMarket() shows wrong @notice. 2022-06-newblockchain\lending-market\contracts\Accountant\AccountantDelegator.sol#L51

• AccountantDelegator.redeemMarket() shows wrong @notice and @param. 2022-06-newblockchain\lending-market\contracts\Accountant\AccountantDelegator.sol#L60-L61

• overflow won't be happened here. In the example contract overflow was possible because last timestamp was modularized but in our case it uses raw timestamp. 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L156

• There is no _mintFee() function in this contract. (Example contract contains it.) 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L246 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L269

Non-critical Issues

1. require()/revert() statements should have descriptive reason strings 2022-06-newblockchain\lending-market\contracts\CNote.sol#L229 2022-06-newblockchain\lending-market\contracts\WETH.sol#L69 2022-06-newblockchain\lending-market\contracts\WETH.sol#L72 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L53 2022-06-newblockchain\lending-market\contracts\Treasury\TreasuryDelegate.sol#L16-L17 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L125 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L285 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L465 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L498 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L503 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L508 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L210-L211 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L456 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L459

2. Event should use indexed fields if there are three or more fields 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L31

3. Typo 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L463 faialure => failure

4. Needless import 2022-06-newblockchain\lending-market\contracts\NoteInterest.sol#L5

5. Immutable addresses should be 0-checked 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L76 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L78

6. Open TODO 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1232 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1271

7. A local variable has the same name as a public one. 2022-06-newblockchain\lending-market\contracts\NoteInterest.sol#L73-L74

1. Use ++i instead of i++, i+=1, also unchecked increments in for-loops will save gas cost 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L126 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L206 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L735 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L959 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1005 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1106 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1347 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1347 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1353 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1364 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L68 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L90 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L207 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L232 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L136 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L362

2. Non-strict inequalities are cheaper than strict ones 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L775 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L522 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L87

3. No need to initialize variables with default values 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L207 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L223-L224 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L136 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L158 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L362 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L126 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L206 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L735 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L959 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1005 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1106 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1347 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1347 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1353 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1364 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1413 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L68 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L90

4. Change storage to memory if possible 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L105 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L116

5. Reduce the size of error message(32 bytes) There are 70 cases in total and I will show in CNote.sol only. 2022-06-newblockchain\lending-market\contracts\CNote.sol#L16 2022-06-newblockchain\lending-market\contracts\CNote.sol#L43 2022-06-newblockchain\lending-market\contracts\CNote.sol#L45 2022-06-newblockchain\lending-market\contracts\CNote.sol#L54 2022-06-newblockchain\lending-market\contracts\CNote.sol#L77 2022-06-newblockchain\lending-market\contracts\CNote.sol#L114 2022-06-newblockchain\lending-market\contracts\CNote.sol#L130 2022-06-newblockchain\lending-market\contracts\CNote.sol#L146 2022-06-newblockchain\lending-market\contracts\CNote.sol#L198 2022-06-newblockchain\lending-market\contracts\CNote.sol#L214 2022-06-newblockchain\lending-market\contracts\CNote.sol#L264 2022-06-newblockchain\lending-market\contracts\CNote.sol#L310 2022-06-newblockchain\lending-market\contracts\CNote.sol#L330

6. Don't use storage value if you can use local variable instead 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L856 closeFactorMantissa => newCloseFactorMantissa

2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1045 pauseGuardian => NewPauseGuardian

2022-06-newblockchain\lending-market\contracts\NoteInterest.sol#L127 baseRatePerYear => newBaseRatePerYear

2022-06-newblockchain\lending-market\contracts\NoteInterest.sol#L144 baseRatePerYear => newBaseRateMantissa

2022-06-newblockchain\lending-market\contracts\NoteInterest.sol#L157 adjusterCoefficient => newAdjusterCoefficient

2022-06-newblockchain\lending-market\contracts\NoteInterest.sol#L170 updateFrequency => newUpdateFrequency

7. An array’s length should be cached to save gas in for-loops 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L42 unigovProposal.targets.length should be cached.

2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L735 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L959 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1106 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1347 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1347 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1353 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1364 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L68 2022-06-newblockchain\lending-market\contracts\Governance\GovernorBravoDelegate.sol#L90 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L207 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L136 2022-06-newblockchain\stableswap\contracts\BaseV1-periphery.sol#L362

8. Add additional require() to save gas cost 2022-06-newblockchain\stableswap\contracts\BaseV1-core.sol#L210 require(granularity != 0) should be added as it will revert after all calculations in this case.

9. Use != 0 instead of > 0 for uint variables There are more than 50 cases and I will show in Comptroller.sol only. 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L309 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L329 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L380 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1129 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1194 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1197 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1200 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1215 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1218 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1221 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1311 2022-06-newblockchain\lending-market\contracts\Comptroller.sol#L1379