Canto v2 contest - zzzitron's results

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/CErc20Delegator.sol#L237 https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/CErc20Delegator.sol#L246

Vulnerability details

Impact

When using CToken implementation with CErc20Delegator, the functions borrowRatePerBlock and supplyRatePerBlock will revert when the underlying functions try to update some states.

Detail

The v1 of borrowRatePerBlock and supplyRatePerBlock were view functions, but they are not anymore. The CErc20Delegator is still using delegateToViewImplementation for those functions. Those functions can be used, as long as the implementation does not update any state variables, i.e. the block number increase since the last update is less or equal to the updateFrequency. However, when these functions are called after sufficient blocks are mined, they are going to revert. Although one can still call the implementation using delegateToImplementation, it is not a good usability, especially if those functions are used for external user interface.

Proof Of Concept

gist for the test

The gist shows a simple test. It calls borrowRatePerBlock and supplyRatePerBlock first time, it suceeds. Then, it mines for more than 300 times, which is the updateFrequency parameter. Then it calls again then fails.

Notes on the test file:

• The setup is taken from tests/Treasury/Accountant.test.ts
• using solidity from ethereum-waffle for chai to use reverted

  // in hardhat.config.js
  import chai from "chai";
  import { solidity } from "ethereum-waffle";
  
  chai.use(solidity);
  

Tools Used

hardhat

Recommended Mitigation Steps

Instead of using delegateToViewImplementation use delegateToImplementation. Alternatively, implement view functions to query these rates in NoteInterest.sol and CToken.sol. It will enable to query the rates without spending gas.

NewBlockChain QA Report

Summary / impression

• The codebase was improved compared to the version 1.

Low

L00: price not scaled by the BASE in BaseV1Router01

• BaseV1-periphery.sol:501

        else if (compareStrings(ctoken.symbol(), "cNOTE") && msg.sender == Comptroller) {
            return 1; // Note price is fixed to 1
        }
  

The getUnderlyingPrice function returns 1, when Comptroller is calling with CNote. The Comptroller is using the given price as mantissa, which has 18 decimal precision.

L01: _acceptInitialAdminDelegated will revert in GovernorBravoDelegator

• GovernorBravoDelegator.sol:49

        delegateTo(implementation, abi.encodeWithSignature("_acceptInitialAdmin()"));
  

When the GovernorBravoDelegator is used with GovernorBravoDelegate as the implementation, the function _acceptInitialAdminDelegated will revert since the GovernorBravoDelegate does not have _acceptInitialAdmin function.

L02: desirable underflow / overflow in BaseV1Pair

• BaseV1-core.sol:160-161

• BaseV1-core.sol:186-187

• BaseV1-core.sol:200-201

• BaseV1-core.sol:232-233

            reserve0CumulativeLast += _reserve0 * timeElapsed;
            reserve1CumulativeLast += _reserve1 * timeElapsed;
  

The BaseV1-core is using solidity version 0.8.11, so it will revert when overflow or underflow happens. Upon cumulating the time weighted reserve, it is desired to overflow. Likewise it is desired to underflow calculating price average. It also saves gas usage, to use unchecked block for those calculations.

L03: Lack of zero address check

• NoteInterest.sol:104-105 in initialize

         cNote = CErc20(cnoteAddr);
         oracle = PriceOracle(oracleAddress);
  

  • the cNote and oracle was set without checking zero address

• NoteInterest.sol:113 in setAdmin

        admin = newAdmin;
  

  • it is good to check admin for zero address, otherwise admin access will be lost forever. To renounce the admin, it is safer to add a separate function.

L04: Oracle failure check in NoteRateModel

• NoteInterest.sol:145

            uint twapMantissa = oracle.getUnderlyingPrice(cNote); // returns price as mantissa
  

It is good to check the returned value from oracle is sane (i.e. if it is zero) and prepare for a fallback.

L05: commented out deadline check in BaseV1Router01

• BaseV1-periphery.sol:87

    modifier ensure(uint deadline) {
        //require(deadline >= block.timestamp, "BaseV1Router: EXPIRED");
  

The deadline is used to prevent the transaction to be withhold and to be used later. However the modifier's line to check the deadline is commented out.

Non-critical

N00: Cast to int without checking in NoteRateModel

• NoteInterest.sol:147-149

            int diff = BASE - int(twapMantissa); //possible annoyance if 1e18 - twapMantissa > 2**255, differ
            int InterestAdjust = (diff * int(adjusterCoefficient))/BASE;
            int ir = InterestAdjust + int(baseRatePerYear);

The uint256 variables are casted into int without checking for overflow. It is probably safe in this use case, but to be sure one may use SafeCast.

N01: storage for delegate is outside of storage contract in CNote

• CNote.sol:14

When updating an implementation for a proxy, it is important to keep the storage structure. Putting non-constant state variables in a dedicated storage contract is less error-prone approach. Although CErc20Delegate has a storage contract in its inheritance tree, a state variable _accountant was introduced in CNote. Therefore, extra care should be paid to the state variable.

N02: Meaningless check in GovernorBravoDelegate

• GovernorBravoDelegate.sol:129

The check require(initialProposalId == 0, "GovernorBravo::_initiate: can only initiate once"); is not useful, as initialProposalId is never updated.

N03: Missing param in NatSpec in CToken

• CToken.sol:24

Although a new argument uint8 stable_ was added, the NatSpec for the parameter is missing.

N04: Misleading NatSpec in AccountantDelegator

• AccountantDelegator.sol:50 the NatSpec does not match the function
• AccountantDelegator.sol:59 the NatSpec does not match the function

N05: Typos

• CToken.sol: "efore" to "before"
• CNote.sol: "efore" to "before"