Canto v2 contest - m_Rassska's results

Table of contents

• [0x0] Disclaimer
• [G-01] Mark functions as external instead of public
• [G-02] Define Custom Errors with error-codes
• [G-03] Cache the data into the memory
• [G-04] Redundant code
• [G-05] Minor optimizations

<br/>

Disclaimer<a name="0x0"></a>

• Please, consider everything described below as a general recommendation. These notes will represent potential possibilities to optimize gas consumption. It's okay, if something is not suitable in your case. Just let me know the reason in the comments section. Enjoy!

<br/>

[G-01] Mark functions as external instead of public <a name="G-01"></a>

PROBLEM

• External functions allow to read directly from calldata, meanwhile public functions need to allocate the space in the memory for arguments.

PROOF OF CONCEPT

  - setAccountantContract(address) should be declared external 
    contracts/CNote.sol#L16-L21

  - RetAccountant() should be declared external
    contracts/Note.sol#L17-L19

  - delegateToViewImplementation(bytes) should be declared external
    contracts/Accountant/AccountantDelegator.sol#L107-L115

  - initialize(address) should be declared external
    contracts/Treasury/TreasuryDelegate.sol#L19-L27

  - delegate(address) should be declared external
    contracts/Governance/Comp.sol#L148-L150

  - delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32) should be declared external
    contracts/Governance/Comp.sol#L161-L170

  - getPriorVotes(address,uint256) should be declared external
     contracts/Governance/Comp.sol#L189-L221 

  - getActions(uint256) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L218-L221GovernorAlpha.getActions(uint256)

  - castVote(uint256,bool) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L249-L251

  - castVoteBySig(uint256,bool,uint8,bytes32,bytes32) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L253-L260

  - __queueSetTimelockPendingAdmin(address,uint256) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L292-L295

  - cancel(uint256) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L203-L216

  - queue(uint256) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L177-L186

  - __abdicate() should be declared external:
    contracts/Governance/GovernorAlpha.sol#L287-L290

  - execute(uint256) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L193-L201

  - propose(address[],uint256[],string[],bytes[],string) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L136-L175

  - __executeSetTimelockPendingAdmin(address,uint256) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L297-L300

  - getReceipt(uint256,address) should be declared external:
    contracts/Governance/GovernorAlpha.sol#L223-L225

  - __acceptAdmin() should be declared external
    contracts/Governance/GovernorAlpha.sol#L282-L285

  - delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32) should be declared external:
    contracts/Governance/Comp.sol#L161-L170

  - setAccountantContract(address) should be declared external:
    contracts/CNote.sol#L16-L21

  - initialize(address) should be declared external:
    contracts/Treasury/TreasuryDelegate.sol#L19-L27

  - initialize(address) should be declared external:
    contracts/Governance/GovernorBravoDelegate.sol#L24-L31

  - drip() should be declared external:
    contracts/Reservoir.sol#L46-L67

  - RetAccountant() should be declared external:
    contracts/Note.sol#L17-L19

  - delegateToViewImplementation(bytes) should be declared external:
    contracts/Accountant/AccountantDelegator.sol#L107-L115

TOOLS USED

• Slither
• Manual Analysis

MITIGATION

• Replace public by external.

<br/>

[G-02] Define Custom Errors with error-codes <a name="G-02"></a>

PROBLEM

• Using predefined custom error leads to gas optimization. Also it's great to revert predefined error-code for the specific message instead of reverting exactly the same message for multiple require statements.

PROOF OF CONCEPT

• contracts/CNote.sol

    - require(msg.sender == admin, "CNote::_setAccountantContract:Only admin may call this function");
    - require(success, "TOKEN_TRANSFER_IN_FAILED");
    - require(balanceCur == 0, "Accountant has not been correctly supplied");
    - require(success, "TOKEN_TRANSFER_OUT_FAILED");
    - require(token.balanceOf(address(this)) == 0, "cNote::doTransferOut: TransferOut Failed");
    - require(_notEntered, "re-entered");
    - require(_notEntered, "re-entered");

• contracts/CToken.sol

    - require(msg.sender == admin, "only admin may initialize the market");
    - require(accrualBlockNumber == 0 && borrowIndex == 0, "market may only be initialized once");
    - require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");
    - require(err == NO_ERROR, "setting comptroller failed");
    - require(err == NO_ERROR, "setting interest rate model failed");
    - require(borrowRateMantissa <= borrowRateMaxMantissa, "borrow rate is absurdly high");
    - require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero");
    - require(amountSeizeError == NO_ERROR, "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");
    - require(cTokenCollateral.balanceOf(borrower) >= seizeTokens, "LIQUIDATE_SEIZE_TOO_MUCH");
    - require(cTokenCollateral.seize(liquidator, borrower, seizeTokens) == NO_ERROR, "token seizure failed");
    - require(newComptroller.isComptroller(), "marker method returned false");

• contracts/NoteInterest.sol

    - require(msg.sender == admin, "only the admin may set the base rate");
    - require(msg.sender == admin, "only the admin may set the adjuster coefficient");
    - require(msg.sender == admin, "only the admin may set the update frequency");

• contracts/Accountant/AccountantDelegate.sol

    - require(cNoteAmt >= noteDiff, "AccountantDelegate::sweepInterest:Error calculating interest to sweep");
    - require(cnote.balanceOf(treasury) == 0, "AccountantDelegate::sweepInterestError");
  

• contracts/Accountant/AccountantDelegator.sol

    - require(msg.sender == admin, "AccountantDelegator::_setImplementation: admin only");
    - require(implementation_ != address(0), "AccountantDelegator::_setImplementation: invalid implementation address");
    - require(msg.value == 0,"AccountantDelegator:fallback: cannot send value to fallback");

• contracts/Comp.sol

    - require(signatory != address(0), "Comp::delegateBySig: invalid signature");
    - require(nonce == nonces[signatory]++, "Comp::delegateBySig: invalid nonce");
    - require(block.timestamp <= expiry, "Comp::delegateBySig: signature expired");
    - require(blockNumber < block.number, "Comp::getPriorVotes: not yet determined");
    - require(src != address(0), "Comp::_transferTokens: cannot transfer from the zero address");
    - require(dst != address(0), "Comp::_transferTokens: cannot transfer to the zero address");

• Following contracts also contains optimization possibilities.

  • contracts/GovernorAlpha.sol
  • contracts/GovernorBravoDelegate.sol
  • contracts/GovernorBravoDelegator.sol
  • contracts/BaseV1-core.sol
  • contracts/BaceV1-periphery.sol
  • contracts/Token.sol
  • contracts/TreasuryDelegator.sol

TOOLS USED

• Slither
• Manual Analysis

MITIGATION

• Define your own custom errors and revert them instead of directly provided revert-message. I would probably create my own library for errors, but it's up to you.

• It's also great to use error-codes, like: "L1" or "P1, P2", alowing fetching this codes with actual messages off-chain.

  <br/>

[G-03] Cache the data into the memory <a name="G-03"></a>

PROBLEM

• Accesing the storage is pretty expensive. Therefore, it's better to cache state variables before proceding these type of operations.

PROOF OF CONCEPT

• contracts/GovernorAlpha.sol

  Maybe, it's better to push this proposal into the memory rather than operating with references? I undestood the motivation behind the reference to the state var, simply you want to be able to change some attributes, but if you want to read those attributes, it's better to load directly from the memory, because SLOAD >> MLOAD. It's sagnificant gas saving, especially in the loops.

    - // Lines: [179-183]
      Proposal storage proposal = proposals[proposalId];
      uint eta = add256(block.timestamp, timelock.delay());
  
      for (uint i = 0; i < proposal.targets.length; i++) {
          _queueOrRevert(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], eta);
      }  
  
    - // Lines: [195-199]    
        Proposal storage proposal = proposals[proposalId];
        proposal.executed = true;
        for (uint i = 0; i < proposal.targets.length; i++) {
            timelock.executeTransaction{value: proposal.values[i]}(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
        }
  
    - // Lines: [207-213]
        Proposal storage proposal = proposals[proposalId];
        proposal.executed = true;
        for (uint i = 0; i < proposal.targets.length; i++) {
            timelock.executeTransaction{value: proposal.values[i]}(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
        }    
    - // Lines: [218-221]
        function getActions(uint proposalId) public view returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas) {
            Proposal storage p = proposals[proposalId];
            return (p.targets, p.values, p.signatures, p.calldatas);
        }
  
    - // Lines: [227-247]
        function state(uint proposalId) public view returns (ProposalState) {
          require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
          Proposal storage proposal = proposals[proposalId];
          if (proposal.canceled) {
              return ProposalState.Canceled;
          } else if (block.number <= proposal.startBlock) {
              return ProposalState.Pending;
          } else if (block.number <= proposal.endBlock) {
              return ProposalState.Active;
          } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes()) {
              return ProposalState.Defeated;
          } else if (proposal.eta == 0) {
              return ProposalState.Succeeded;
          } else if (proposal.executed) {
              return ProposalState.Executed;
          } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) {
              return ProposalState.Expired;
          } else {
              return ProposalState.Queued;
          }
        }
    - // Lines: [262-280]
        function _castVote(address voter, uint proposalId, bool support) internal {
          require(state(proposalId) == ProposalState.Active, "GovernorAlpha::_castVote: voting is closed");
          Proposal storage proposal = proposals[proposalId];
          Receipt storage receipt = proposal.receipts[voter];
          require(receipt.hasVoted == false, "GovernorAlpha::_castVote: voter already voted");
          uint96 votes = comp.getPriorVotes(voter, proposal.startBlock);
  
          if (support) {
              proposal.forVotes = add256(proposal.forVotes, votes);
          } else {
              proposal.againstVotes = add256(proposal.againstVotes, votes);
          }
  
          receipt.hasVoted = true;
          receipt.support = support;
          receipt.votes = votes;
  
          emit VoteCast(voter, proposalId, support, votes);
        }
  
    - // Lines: [287-290]
        function __abdicate() public {
          require(msg.sender == guardian, "GovernorAlpha::__abdicate: sender must be gov guardian");
          guardian = address(0);
        }
  
    - // Lines: [292-295]
        function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public {
          require(msg.sender == guardian, "GovernorAlpha::__queueSetTimelockPendingAdmin: sender must be gov guardian");
          timelock.queueTransaction(address(timelock), 0, "setPendingAdmin(address)", abi.encode(newPendingAdmin), eta);
        }
    
    - // Lines: [297-300]
        function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public {
          require(msg.sender == guardian, "GovernorAlpha::__executeSetTimelockPendingAdmin: sender must be gov guardian");
          timelock.executeTransaction(address(timelock), 0, "setPendingAdmin(address)", abi.encode(newPendingAdmin), eta);
        }

  • Also, these contracts contains the same possibilities:
    • contracts/CToken.sol
    • contracts/Governance/GovernorBravoDelegate.sol

TOOLS USED

• Slither
• Manual Analysis

MITIGATION

• For those cases, where you would like to change the state var, it's okay to define it as storage. But for loads define an object in the memory, then operate with it.

<br/>

[G-04] Redundant code <a name="G-04"></a>

PROBLEM

• There is no need to check for overflow and underflows since solidity 0.8.0.
• There is no need in all custom arithmetic operations since solidity 0.8.0.

PROOF OF CONCEPT

• contracts/Reservoir.sol

  - // Lines: [57-60]
      uint dripTotal_ = mul(dripRate_, blockNumber_ - dripStart_, "dripTotal overflow");
      uint deltaDrip_ = sub(dripTotal_, dripped_, "deltaDrip underflow");
      uint toDrip_ = min(reservoirBalance_, deltaDrip_);
      uint drippedNext_ = add(dripped_, toDrip_, "tautological");

• contracts/Reservoir.sol

  - // Lines: [71-101]
    function add(uint a, uint b, string memory errorMessage) internal pure returns (uint) {
      uint c;
      unchecked { c = a + b; }
      require(c >= a, errorMessage);
      return c;
    }

    function sub(uint a, uint b, string memory errorMessage) internal pure returns (uint) {
      require(b <= a, errorMessage);
      uint c = a - b;
      return c;
    }

    function mul(uint a, uint b, string memory errorMessage) internal pure returns (uint) {
      if (a == 0) {
        return 0;
      }
      uint c;
      unchecked { c = a * b; }
      require(c / a == b, errorMessage);
      return c;
    }

    function min(uint a, uint b) internal pure returns (uint) {
      if (a <= b) {
        return a;
      } else {
        return b;
      }
    }

• contracts/Governance/GovernorAlpha.sol

- // Lines: [302-311]
      function add256(uint256 a, uint256 b) internal pure returns (uint) {
        uint c = a + b;
        require(c >= a, "addition overflow");
        return c;
      }

      function sub256(uint256 a, uint256 b) internal pure returns (uint) {
          require(b <= a, "subtraction underflow");
          return a - b;
      }

• contracts/Governance/GovernorBravoDelegate.sol

- // Lines: [174-183]
  function add256(uint256 a, uint256 b) internal pure returns (uint) {
      uint c = a + b;
      require(c >= a, "addition overflow");
      return c;
  }

  function sub256(uint256 a, uint256 b) internal pure returns (uint) {
      require(b <= a, "subtraction underflow");
      return a - b;
  }

TOOLS USED

• Slither
• Manual Analysis

MITIGATION

• I consider that as a redundant code, needs to be removed and handled properly.

<br/>

[G-05] Minor optimizations <a name="G-05"></a>

General Recommendations

• Use:
  • [G-05.1] Unchecked in order to pass overflow/underflow extra checks, when you can prove the opposite.
  • [G-05.2] There is no need to write dripped = 0 or uint i = 0, it's == 0 by default. It consumes gas for MLOAD/SLOAD opcodes
  • [G-05.3] Use ++i, instead of i++. It's just a magic =)
  • [G-05.4] Break down complex requires. require(a == b && c == d) => require(a == b); require(c == d), extra gas usage for AND opcode.
  • [G-05.6] Replace reccuring reverts if (msg.sender != admin){revert ...} with modifiers, reduces deployment cost.
  • [G-05.7] There is no need to cast uint256 => uint96/uint32, if it's not properly tide up in a storage. It costs extra gas for casting.
  • [G-05.8] Inline internal functions, if they are called only once.
  • [G-05.9] .balance see EIP-2929.