Canto contest - gzeon's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/NoteInterest.sol#L95-L96

Vulnerability details

Impact

The random number used in NoteInterest.getBorrowRate is easily bruteforceable by an attacker to generate the address that give the lowest interest rate.

Proof of Concept

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/NoteInterest.sol#L95-L96

        uint rand = uint(keccak256(abi.encodePacked(msg.sender))) % 100;
        uint ir = (100 - rand).mul(adjusterCoefficient).add(baseRatePerYear).mul(1e16);

Recommended Mitigation Steps

Use blockhash as the random source

Lines of code

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

Vulnerability details

Impact

repayAmount was used instead of repayAmountFinal in CNote.repayBorrowFresh. This will lead to revert if the borrower want to repay full amount with type(uint).max

Proof of Concept

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

        uint actualRepayAmount = doTransferIn(payer, repayAmount);

Recommended Mitigation Steps

        uint actualRepayAmount = doTransferIn(payer, repayAmountFinal);

Low

Shadowed nonces variable

nonces is defined in ERC20Data https://github.com/Plex-Engineer/zeroswap/blob/03507a80322112f4f3c723fc68bed0f138702836/contracts/SushiToken.sol#L120

Use require instead of assert

Assert false should not be triggered in production as it will consume all remaining gas, use require instead.

contracts/BaseV1-periphery.sol:82:        assert(msg.sender == address(wcanto)); // only accept ETH via fallback from the WETH contract
contracts/BaseV1-periphery.sol:227:                assert(amountAOptimal <= amountADesired);
contracts/BaseV1-periphery.sol:273:        assert(wcanto.transfer(pair, amountCANTO));
contracts/BaseV1-periphery.sol:419:        assert(wcanto.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]));

Multiple pragma used

Consider to use a single pragma config

lending-market/contracts/EIP20NonStandardInterface.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/ComptrollerInterface.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Note.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Comptroller.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CErc20.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/ERC20.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/PriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/ComptrollerG7.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CToken.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CErc20Delegator.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CErc20Delegate.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CDaiDelegate.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/ExponentialNoError.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/SafeMath.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/ErrorReporter.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Unitroller.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/NoteInterest.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/JumpRateModel.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Timelock.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/EIP20Interface.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/InterestRateModel.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/JumpRateModelV2.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/ComptrollerStorage.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CNote.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Lens/CompoundLens.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/WhitePaperInterestRateModel.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Reservoir.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Accountant/AccountantDelegator.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Accountant/AccountantInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Accountant/AccountantDelegate.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/WETH.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorAlpha.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorBravoInterfaces.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorBravoDelegate.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/Comp.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Governance/GovernorBravoDelegator.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CErc20Immutable.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CEther.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/DAIInterestRateModelV3.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/Treasury/TreasuryDelegator.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Treasury/TreasuryInterfaces.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Treasury/TreasuryDelegate.sol:1:pragma solidity ^0.8.10;
lending-market/contracts/Maximillion.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/SimplePriceOracle.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/BaseJumpRateModelV2.sol:2:pragma solidity ^0.8.10;
lending-market/contracts/CTokenInterfaces.sol:2:pragma solidity ^0.8.10;
zeroswap/contracts/mocks/RewarderMock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/WETH9Mock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/SushiSwapPairMock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/SushiSwapFactoryMock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/ERC20Mock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/ComplexRewarderTime.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/ComplexRewarder.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/SushiMakerKashiExploitMock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/SushiMakerExploitMock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/mocks/RewarderBrokenMock.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/Migrator.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/MiniChefV2.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/SushiRoll.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/uniswapv2/UniswapV2Factory.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/UniswapV2ERC20.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/libraries/UniswapV2OracleLibrary.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/libraries/UniswapV2Library.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/libraries/SafeMath.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/libraries/UQ112x112.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/libraries/Math.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/libraries/TransferHelper.sol:3:pragma solidity >=0.6.0;
zeroswap/contracts/uniswapv2/UniswapV2Oracle.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/UniswapV2Router02.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/UniswapV2Pair.sol:3:pragma solidity =0.6.12;
zeroswap/contracts/uniswapv2/interfaces/IERC20.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/interfaces/IUniswapV2Router01.sol:3:pragma solidity >=0.6.2;
zeroswap/contracts/uniswapv2/interfaces/IUniswapV2ERC20.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/interfaces/IUniswapV2Router02.sol:3:pragma solidity >=0.6.2;
zeroswap/contracts/uniswapv2/interfaces/IWETH.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/interfaces/IUniswapV2Factory.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/interfaces/IUniswapV2Pair.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/uniswapv2/interfaces/IUniswapV2Callee.sol:3:pragma solidity >=0.5.0;
zeroswap/contracts/SushiBar.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/bentobox/BentoBoxV1.sol:19:pragma solidity 0.6.12;
zeroswap/contracts/bentobox/PeggedOracleV1.sol:2:pragma solidity 0.6.12;
zeroswap/contracts/bentobox/KashiPairMediumRiskV1.sol:18:pragma solidity 0.6.12;
zeroswap/contracts/SushiMakerKashi.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/Ownable.sol:5:pragma solidity 0.6.12;
zeroswap/contracts/libraries/SafeERC20.sol:2:pragma solidity 0.6.12;
zeroswap/contracts/libraries/SafeMath.sol:2:pragma solidity 0.6.12;
zeroswap/contracts/libraries/SignedSafeMath.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/MasterChefV2.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/SushiToken.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/MasterChef.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/SushiMaker.sol:4:pragma solidity 0.6.12;
zeroswap/contracts/governance/Timelock.sol:13:// XXX: pragma solidity ^0.5.16;
zeroswap/contracts/governance/Timelock.sol:14:pragma solidity 0.6.12;
zeroswap/contracts/interfaces/IERC20.sol:2:pragma solidity 0.6.12;
zeroswap/contracts/interfaces/IRewarder.sol:3:pragma solidity 0.6.12;
zeroswap/contracts/interfaces/IMasterChef.sol:2:pragma solidity 0.6.12;

Non-Critical

Typo

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/CToken.sol#L290 efore -> before

Hardcoded address

https://github.com/Plex-Engineer/lending-market/blob/2d423c7c3f62d65182d802deb99cc7bba4e057fd/contracts/Governance/GovernorBravoDelegate.sol#L28

        unigov = IProposal(0x30E20d0A642ADB85Cb6E9da8fB9e3aadB0F593C0);

Use 1 and 2 in nonReentrant modifier to save gas

see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/83277ff916ac4f58fec072b8f28a252c1245c2f1/contracts/security/ReentrancyGuard.sol#L29-33

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/CToken.sol#L1151-L1156

    modifier nonReentrant() virtual {
        require(_notEntered, "re-entered");
        _notEntered = false;
        _;
        _notEntered = true; // get a gas-refund post-Istanbul
    }

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590

contracts/CToken.sol:37:        require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");

For loop optimization unchecked increment

Using for(...){...unchecked{++i;}} can save gas

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/