Canto v2 contest - 0x1f8b's results

Lines of code

https://github.dev/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/CNote.sol#L148

Vulnerability details

Impact

The CNote.doTransferOut method is susceptible to denial of service.

Proof of Concept

The logic of the doTransferOut method in CNote is as follows:

    function doTransferOut(address payable to, uint amount) virtual override internal {
        require(address(_accountant) != address(0));
        EIP20Interface token = EIP20Interface(underlying);
        if (to != address(_accountant)) {
            uint err = _accountant.supplyMarket(amount);
            if (err != 0) { revert AccountantRedeemError(amount); }
        }   
        token.transfer(to, amount);
        bool success;
        assembly {
            switch returndatasize()
                case 0 { success := not(0) }
                case 32 { 
                    returndatacopy(0, 0, 32)
                    success := mload(0)
                }
                default { revert(0, 0) }
        } 
        require(success, "TOKEN_TRANSFER_OUT_FAILED");
        require(token.balanceOf(address(this)) == 0, "cNote::doTransferOut: TransferOut Failed"); // <-- ERROR
    }

The doTransferOut method receives an amount which is transferred to to, after it the balance of the contract token is checked to be equal to zero or the transaction will be reverted.

In the following cases a denial of service will occur:

• In the case that is used an amount different than the balance, the transaction will be reverted.
• In the case that an attacker front-runs the transaction and sends one token more than the established by the _accountant.
• In case of increasing balance tokens like mDai that constantly change their balance, the established by the _accountant will be different when the transaction is persisted.

Recommended Mitigation Steps

• Use balance differences instead of the 0 check.

Lines of code

https://github.dev/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/Accountant/AccountantDelegate.sol#L101

Vulnerability details

Impact

The sweepInterest method is susceptible to denial of service.

Proof of Concept

The logic of the sweepInterest method relative to the treasury is as follows:

		bool success = cnote.transfer(treasury, amtToSweep);
		if (!success) { revert  SweepError(treasury , amtToSweep); }
		TreasuryInterface Treas = TreasuryInterface(treasury);
		Treas.redeem(address(cnote),amtToSweep);
		require(cnote.balanceOf(treasury) == 0, "AccountantDelegate::sweepInterestError");

As you can see, amtToSweep is passed to it and redeem that amount. Later it is checked that the balance of cnote in the treasury address must be 0. However, all calculations related to amtToSweep come out of the balance of address(this) so if a third party sends a single token cnote to the address of treasury the method will be denied.

Recommended Mitigation Steps

• Check that the balance is the same after and before the bool success = cnote.transfer(treasury, amtToSweep);

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/2646a7676b721db8a7754bf5503dcd712eab2f8a/contracts/NoteInterest.sol#L99

Vulnerability details

Impact

The initialize method of the contract NoteInterest can be initialized multiple times.

Proof of Concept

The method initialize of the contract NoteInterest looks like this:

    function initialize(address cnoteAddr, address oracleAddress) external {
        if (msg.sender != admin ) {
            revert SenderNotAdmin(msg.sender);
        }   
        address oldPriceOracle = address(oracle);
        cNote = CErc20(cnoteAddr);
        oracle = PriceOracle(oracleAddress);
        emit NewPriceOracle(oldPriceOracle, oracleAddress);
    }

Nothing prevents it from being initialized again and altering the initial values of the contract. This allows the government, unnecessarily, to be able to perform attacks such as altering the logic of the updateBaseRate method.

Recommended Mitigation Steps

• And a require to check that was not already initialized.

• Low
  • 1. Outdated compiler
  • 2. Upgradable contracts without GAP can lead a upgrade disaster
  • 3. TreasuryDelegator don't allow to change receive logic
  • 4. Call to external contract without authentication
  • 5. Lack of checks
  • 6. Unsecure state order
  • 7. Use of transfer instead of call
  • 8. Lack of ACK during owner change
  • 9. Owner can deny the service
  • 10. Unsafe ERC20 calls
• Non-Critical
  • 11. Use abstract for base contracts
  • 12. Use hardcoded values

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.10;
pragma solidity ^0.8.0;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

Examples:

• AccountantDelegate.sol#L1
• ERC20Burnable.sol#L4

2. Upgradable contracts without GAP can lead a upgrade disaster

Some contract does not implement a good upgradeable logic.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, if it is an interface, you have to use the interface keyword, otherwise it is a contract that requires your GAP to be correctly updated, so if a new variable is created it could lead in storage collisions that will produce a contract dissaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

Affected source code:

• AccountantErrors
• AccountantDelegatorStorage
• AccountantStorageV1
• AccountantDelegatorInterface
• AccountantInterface
• TreasuryDelegatorStorage
• TreasuryStorageV1
• TreasuryDelegatorInterface
• TreasuryInterface
• GovernorBravoEvents
• GovernorBravoDelegatorStorage
• GovernorBravoDelegateStorageV1
• GovernorBravoDelegateStorageV2

3. TreasuryDelegator don't allow to change receive() logic

The contract does not send the call to the implementation and therefore cannot be updated by an implementation change.

Affected source code:

• TreasuryDelegator.sol#L136

4. Call to external contract without authentication

It is allowed to call the redeem method without authentication control or whitelist, so the contract could call an external contract on behalf of the treasury, something that is not recommended.

Affected source code:

• TreasuryDelegate.sol#L53

5. Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• GovernorAlpha.sol#L130
• Comp.sol#L64
• Reservoir.sol#L33

There are also integer variables that must be in a certain range that are not checked.

Affected source code:

• NoteInterest.sol#L165
• NoteInterest.sol#L178
• NoteInterest.sol#L191

6. Unsecure state order

Negative states must be processed before positive states, otherwise, a proposal that is Expired or Executed, but is Active or Pending will be returned as Active or Pending, this makes it necessary to check that the state is correct from outside this method. I mean, changing outside the variables that alter this state in the correct way.

Affected source code:

• GovernorAlpha.sol#L227-L247

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;
    }
}

7. Use of transfer instead of call

To transfer ether, the transfer method (which is capped at 2300 gas) is used instead of call which is limited to the gas provided by the user.

If a contract that has a fallback method more expensive than 2300 gas, it will be impossible for a contract receive funds from Basket contract.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• WETH.sol#L31

8. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• Note.sol#L21
• NoteInterest.sol#L113

9. Owner can deny the service

adjusterCoefficient is casted to int and the owner can set a value in _setAdjusterCoefficient that can produce an overflow.

Affected source code:

• NoteInterest.sol#L148

10. Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• EIP-20

• NOTES: The following specifications use syntax from Solidity 0.4.17 (or above). Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Affected source code for transferFrom:

• CNote.sol#L77

Non-Critical

11. Use abstract for base contracts

Abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• AccountantErrors
• AccountantDelegatorStorage
• AccountantStorageV1
• TreasuryDelegatorStorage
• TreasuryStorageV1
• GovernorBravoEvents
• GovernorBravoDelegatorStorage
• GovernorBravoDelegateStorageV1
• GovernorBravoDelegateStorageV2

12. Use hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these addresses from the source code.

Affected source code:

• GovernorBravoDelegate.sol#L28
• ERC20DirectBalanceManipulation.sol#L10)
• ERC20MaliciousDelayed.sol#L10

• Gas
  • 1. Use abi.encodeWithSelector instead of abi.encodeWithSignature
  • 2. Avoid creating unnecessary variables
  • 3. Reorder structure layout
  • 4. Reduce the size of error messages Long revert Strings
    • Use Custom Errors instead of Revert Strings to save Gas
      • Source Custom Errors in Solidity:
  • 5. ++i costs less gas compared to i++ or i += 1
  • 6. Use external visibility
  • 7. There's no need to set default values for variables
  • 8. Reduce boolean comparison
  • 9. Unused arguments
  • 10. Gas saving using immutable
  • 11. Unused import
  • 12. unckecked keyword
  • 13. use constant instead of storage
  • 14. Unrequired method
  • 15. Optimize logic
  • 16. Use memory instead of storage

Gas

1. Use abi.encodeWithSelector instead of abi.encodeWithSignature

abi.encodeWithSelector is much cheaper than abi.encodeWithSignature because it doesn't require to compute the selector from the string.

Reference:

• https://docs.soliditylang.org/en/v0.8.15/units-and-global-variables.html#abi-encoding-and-decoding-functions
• https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

Affected source code:

• AccountantDelegator.sol#L27
• AccountantDelegator.sol#L54
• AccountantDelegator.sol#L63
• AccountantDelegator.sol#L71
• AccountantDelegator.sol#L108
• TreasuryDelegator.sol#L17
• TreasuryDelegator.sol#L45
• TreasuryDelegator.sol#L54
• TreasuryDelegator.sol#L64
• TreasuryDelegator.sol#L68
• TreasuryDelegator.sol#L89
• GovernorBravoDelegator.sol#L15
• GovernorBravoDelegator.sol#L41
• GovernorBravoDelegator.sol#L49
• GovernorBravoDelegator.sol#L57

2. Avoid creating unnecessary variables

The creation of variables has an extra cost due to the required opcodes, in addition to the fact that the evm virtual machine is limited to the number of elements.

Reference:

• https://github.com/ethereum/solidity/issues/2693

Affected source code:

• Return address(this).balance without create treasuryCantoBalance in TreasuryDelegate.sol#L33-L36
• Return note.balanceOf(address(this)) without create treasuryNoteBalance in TreasuryDelegate.sol#L43-L44
• Send msg.sender instead of create owner var in ERC20.sol#L92, ERC20.sol#L116, ERC20.sol#L162 and ERC20.sol#L181
• Send msg.sender instead of create src var in CToken.sol#L145

3. Reorder structure layout

The following structs could be optimized moving the position of certains values in order to save slot storages:

• GovernorBravoInterfaces.sol#L68-L95 and GovernorAlpha.sol#L69-L73

struct Proposal {
    uint id;
    address proposer;
    uint eta;
    address[] targets;
    uint[] values;
    string[] signatures;
    bytes[] calldatas;
    bool canceled;  // <- Move these two bool close to the `proposer` address
    bool executed;
}

4. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Below are some examples, but the code has a lot of them, it is recommended to change all require phrases to custom errors.

• GovernorBravoDelegate.sol#L25-L27
• GovernorBravoDelegate.sol#L46-L47
• GovernorBravoDelegate.sol#L76
• GovernorBravoDelegate.sol#L85
• GovernorAlpha.sol#L137-L146
• GovernorAlpha.sol#L178
• GovernorAlpha.sol#L189
• GovernorAlpha.sol#L194
• GovernorAlpha.sol#L205
• GovernorAlpha.sol#L283-L298
• Comp.sol#L234-L238
• ERC20.sol#L248-L251
• ERC20.sol#L276-L277
• CNote.sol#L17

5. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• GovernorBravoDelegate.sol#L66
• GovernorBravoDelegate.sol#L88
• GovernorAlpha.sol#L181
• GovernorAlpha.sol#L197
• GovernorAlpha.sol#L211

6. Use external visibility

The following methods could be improved if external visibility is used:

• GovernorAlpha.sol#L136

7. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• GovernorBravoDelegate.sol#L66
• GovernorBravoDelegate.sol#L88
• GovernorAlpha.sol#L181
• GovernorAlpha.sol#L197
• GovernorAlpha.sol#L211

8. Reduce boolean comparison

It's compared a boolean value using == true or == false, instead of using the boolean value. NOT opcode, it's cheaper to use EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Change == false by ! in:

• GovernorAlpha.sol#L266

9. Unused arguments

Is not required to send the nonce, only is required for compute the signature.

Affected source code:

• Comp.sol#L161

10. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• Remove _setupDecimals method and change _decimals to immutable. Decimals should never change in ERC20MinterBurnerDecimals.sol#L30
• dripStart, dripRate, token and target in Reservoir.sol#L34-L37
• _name, _symbol and _decimals in WETH.sol#L6-L8
• _name and _symbol in ERC20.sol#L30-L31

11. Unused import

Importing pointless files costs gas during deployment and is a bad coding practice that is important to ignore.

Remove the following imports:

• Reservoir.sol#L103

12. unckecked keyword

It's possible to save gas using the unckecked keyword. This will avoid the required checks to ensure that the variable won't overflow.

Reference:

• https://docs.soliditylang.org/en/v0.8.0/control-structures.html#checked-or-unchecked-arithmetic

The balance was checked before, it can be subtracted without checking overflows:

• WETH.sol#L30
• WETH.sol#L73
• WETH.sol#L76
• ERC20.sol#L213

13. use constant instead of storage

Use constant instead of storage and change from uint256 MAX_INT = 2**256-1; to uint256 private constant MAX_INT = type(uint256).max;:

• ERC20.sol#L18

14. Unrequired method

Method not necessary since the implemented logic only calls to obtain the current block.

Affected source code:

• CToken.sol#L201

15. Optimize logic

The logic of the CToken.transferTokens method can bypass the startingAllowance condition and the allowanceNew variable if the spender and src checks are done at the same time the tokens are subtracted.

if (spender == src) {
    transferAllowances[src][spender] -= tokens;
}

Affected source code:

• CToken.sol#L82

16. Use memory instead of storage

Changing storage to memory we can save some storage access.

Affected source code:

• CToken.sol#L255