Canto v2 contest - Chom's results

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L72

Vulnerability details

Impact

Oracle periodSize = 0 which is as same as not using any oracle. It should be 1800 as expected (30 minutes).

Proof of Concept

// Capture oracle reading every 30 minutes
uint constant periodSize = 0;

You comment said that capture oracle reading every 30 minutes.

But in fact, periodSize = 0 which mean you are updating your oracle every block (TWAP window 1 block) which is as same as not using any oracle.

Tools Used

Diff checker https://www.diffchecker.com/skDZcZJa

Recommended Mitigation Steps

periodSize = 1800;

1800 seconds = 30 minutes

// Capture oracle reading every 30 minutes
uint256 constant periodSize = 1800;

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/443a8c0fed3c5018e95f3881a31b81a555c42b2d/contracts/NoteInterest.sol#L116-L119

Vulnerability details

Impact

getBorrowRate must return rate per block as defined in compound interest rate model but currently return rate per year.

Defined here: https://github.com/compound-finance/compound-protocol/blob/master/contracts/InterestRateModel.sol

Proof of Concept

This is definition

    /**
      * @notice Calculates the current borrow interest rate per block
      * @param cash The total amount of cash the market has
      * @param borrows The total amount of borrows the market has outstanding
      * @param reserves The total amount of reserves the market has
      * @return The borrow rate per block (as a percentage, and scaled by 1e18)
      */
    function getBorrowRate(uint cash, uint borrows, uint reserves) virtual external view returns (uint);

This is implementation

    function getBorrowRate(uint cash, uint borrows, uint reserves) external override returns(uint) {
        updateBaseRate();
        return baseRatePerYear;
    }

Notice that definition require borrow rate per block but implementation return baseRatePerYear which is rate per year

Tools Used

Manual

Recommended Mitigation Steps

return baseRatePerBlock

    function getBorrowRate(uint cash, uint borrows, uint reserves) external override returns(uint) {
        updateBaseRate();
        return baseRatePerBlock;
    }

Use prettier

Your code is not formatted properly, you should install and enforce prettier on all your contract files.

Incorrect comment

https://github.com/Plex-Engineer/manifest-v2/blob/f6ebfe679973edf4f64832e64480ff5250ef8486/contracts/Proposal-Store.sol#L8

// This is an evil token. Whenever an A -> B transfer is called, half of the amount goes to B
// and half to a predefined C
contract ProposalStore {
...
}

ProposalStore is not an evil token!

Remove hardhat/console.sol

It's not a good idea to have debugging code going to the production.

import "hardhat/console.sol";

In these files:

• https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L5
• https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantDelegate.sol#L7
• https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L4

Remove console.log

It's not a good idea to have debugging code going to the production.

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L207

console.log("tokenIn: ", tokenIn);

Use more optimized version of square root

Currently, you are using this version

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L20-L31

    function sqrt(uint y) internal pure returns (uint z) {
        if (y > 3) {
            z = y;
            uint x = y / 2 + 1;
            while (x < z) {
                z = x;
                x = (y / x + x) / 2;
            }
        } else if (y != 0) {
            z = 1;
        }
    }

But there is a more optimized version in PRBMath library

https://github.com/paulrberg/prb-math

/// @notice Calculates the square root of x, rounding down.
/// @dev Uses the Babylonian method https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Babylonian_method.
/// @param x The uint256 number for which to calculate the square root.
/// @return result The result as an uint256.
function sqrt(uint256 x) internal pure returns (uint256 result) {
    if (x == 0) {
        return 0;
    }

    // Calculate the square root of the perfect square of a power of two that is the closest to x.
    uint256 xAux = uint256(x);
    result = 1;
    if (xAux >= 0x100000000000000000000000000000000) {
        xAux >>= 128;
        result <<= 64;
    }
    if (xAux >= 0x10000000000000000) {
        xAux >>= 64;
        result <<= 32;
    }
    if (xAux >= 0x100000000) {
        xAux >>= 32;
        result <<= 16;
    }
    if (xAux >= 0x10000) {
        xAux >>= 16;
        result <<= 8;
    }
    if (xAux >= 0x100) {
        xAux >>= 8;
        result <<= 4;
    }
    if (xAux >= 0x10) {
        xAux >>= 4;
        result <<= 2;
    }
    if (xAux >= 0x8) {
        result <<= 1;
    }

    // The operations can never overflow because the result is max 2^127 when it enters this block.
    unchecked {
        result = (result + x / result) >> 1;
        result = (result + x / result) >> 1;
        result = (result + x / result) >> 1;
        result = (result + x / result) >> 1;
        result = (result + x / result) >> 1;
        result = (result + x / result) >> 1;
        result = (result + x / result) >> 1; // Seven iterations should be enough
        uint256 roundedDownResult = x / result;
        return result >= roundedDownResult ? roundedDownResult : result;
    }
}

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example,

require(msg.sender == admin, "CErc20::sweepToken: only admin can sweep tokens");

Can be replaced with

// declare error before contract declaration
error OnlyAdmin();

if (msg.sender != admin) revert OnlyAdmin();