Canto contest - catchup's results

Lines of code

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

Vulnerability details

Impact

_setAccountantContract() is a public function which sets the _accountant and admin addresses. It is called by the initialize() function of AccountantDelegate.sol. If the _accountant is address(0) - i.e _setAccountantContract() has not been called yet, so _accountant at it's init value of address(0) - (msg.sender == admin) requirement is bypassed.

    function _setAccountantContract(address payable accountant_) public {		//@audit-issue Sets the admin address. It can be frontrunned if (address(_accountant) == address(0))
        if (address(_accountant) != address(0)){
            require(msg.sender == admin, "CNote::_setAccountantContract:Only admin may call this function");
        }
        emit AccountantSet(accountant_, address(_accountant));
	    _accountant = AccountantInterface(accountant_);
        admin = accountant_;
    }

This means, a malicious caller can frontrun and call _setAccountantContract() if address(_accountant) == address(0), hence can acquire admin privileges.

Proof of Concept

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L14-L21 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegate.sol#L37

Tools Used

Manual review

Recommended Mitigation Steps

Frankly, I am not sure why the "if (address(_accountant) != address(0))" condition is needed. I believe this condition can be removed and "msg.sender == admin" requirement can be valid at all times.

Floating pragma and different compiler versions among contracts

Some of the contracts are using floating pragma ^0.8.10, some using floating ^0.8.0, and some use fixed 0.8.11. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. It is recommended to use the same solidity version for all the contracts.

References: https://swcregistry.io/docs/SWC-103 https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used

Missing non-zero address checks when setting important addresses

It is a good practice to include 0 address check while updating an important address. I suggest to include a non-zero address check for the functions below.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1016 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1033 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L14

Each event can have up to three indexed fields.

Many events do not have indexed fields. Consider using more indexed fields for better log filtering.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/CNote.sol#L10 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L40 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L46 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L67 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L70

Typo in comment

contructor --> constructor

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegate.sol#L10

Missing non-zero address checks for token transfers

Tokens would be burned if sent to zero address accidentally. Therefore, it is a good practice to include non-zero address checks for token transfers.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/AssetLogic.sol#L145 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/SponsorVault.sol#L296 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1059

for loops can be optimized

1- For loop index increments can be made unchecked for solidity versions > 0.8.0. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint variable to 0 since default value is already 0.

Lines of code

https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L207 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L337 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L136 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L362 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L126 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L206 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L735 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L959 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1106 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1347 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1359 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1364 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Comptroller.sol#L1413 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L68 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L90

Recommended Mitigation Steps

I suggest to change the original code from this

        for (uint i = 0; i < _prices.length; i++) {
            priceAverageCumulative += _prices[i];
        }

to this

        for (uint i; i < _prices.length; ) {
            priceAverageCumulative += _prices[i];
            unchecked { ++i; }
        }

Error string longer than 32 characters

Error reason strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

There are many instances some of which are: https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegate.sol#L17-L18 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegate.sol#L29 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegate.sol#L48 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Accountant/AccountantDelegate.sol#L60

Using != 0 is cheaper than > 0 when used on a uint in a require() statement with the optimizer enabled

Lines of code

https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L253 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L272 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L286 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L303 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L465 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L104 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L105 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L456 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L463

allMarkets.length can be cached

allMarkets.length is used in the for loop. It can be cached and used from stack to avoid unnecessary SLOAD's.

Lines of code

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

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

All the for loop index inits to zero and additioanlly the lines below. https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L46 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L223-L224 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-periphery.sol#L158

Constant keccak variables can be changed to immutable

When variables are defined as constant, keccak operation will be performed whenever the variable is used. However, if they are immutable, keccak hashing would be performed only during deployment. If the variable is going to be used within the constructor, the value assignment of the immutable variable should be executed within the constructor as well. See below for previous findings. https://github.com/code-423n4/2021-10-slingshot-findings/issues/3 https://github.com/code-423n4/2022-01-livepeer-findings/issues/172

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L15 https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L18