Canto contest - Chom's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/NoteInterest.sol#L118-L129

Vulnerability details

Impact

Everyone can call updateBaseRate. Note's interest rate may be infinite.

There aren't any permission check.

Proof of Concept

    /**
     * @notice Updates the Note's base rate per year at a given interval
     * @notice This function should be called at a given interval (TBD)
     * @param newBaseRatePerYear The new base rate per year of Note
     */
    function updateBaseRate(uint newBaseRatePerYear) public {
        // check the current block number
        uint blockNumber = block.number;
        uint deltaBlocks = blockNumber.sub(lastUpdateBlock);

        if (deltaBlocks > updateFrequency) {
            // pass in a base rate per year
            baseRatePerYear = newBaseRatePerYear;
            lastUpdateBlock = blockNumber;
            emit NewInterestParams(baseRatePerYear);
        }
    }

updateBaseRate is public

No permission check

No limit check

Everyone can input newBaseRatePerYear = type(uint256).max to destroy the protocol by setting Note's interest rate to infinite.

Tools Used

Manual

Recommended Mitigation Steps

Add something like require(msg.sender == admin, "only the admin may set the base rate"); to the function

    /**
     * @notice Updates the Note's base rate per year at a given interval
     * @notice This function should be called at a given interval (TBD)
     * @param newBaseRatePerYear The new base rate per year of Note
     */
    function updateBaseRate(uint newBaseRatePerYear) public {
        require(msg.sender == admin, "only the admin may set the base rate");

        // check the current block number
        uint blockNumber = block.number;
        uint deltaBlocks = blockNumber.sub(lastUpdateBlock);

        if (deltaBlocks > updateFrequency) {
            // pass in a base rate per year
            baseRatePerYear = newBaseRatePerYear;
            lastUpdateBlock = blockNumber;
            emit NewInterestParams(baseRatePerYear);
        }
   

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/b93e2867a64b420ce6ce317f01c7834a7b6b17ca/contracts/NoteInterest.sol#L92-L101

Vulnerability details

Impact

Borrow rate is random. It shouldn't be random. Did you forgot to implement this part?

Proof of Concept

    function getBorrowRate(uint cash, uint borrows, uint reserves) public view override returns (uint) {
        // Gets the Note/gUSDC TWAP in a given interval, as a mantissa (scaled by 1e18)
        // uint twapMantissa = getUnderlyingPrice(note);
        uint rand = uint(keccak256(abi.encodePacked(msg.sender))) % 100;
        uint ir = (100 - rand).mul(adjusterCoefficient).add(baseRatePerYear).mul(1e16);
        uint newRatePerYear = ir >= 0 ? ir : 0;
        // convert it to base rate per block
        uint newRatePerBlock = newRatePerYear.div(blocksPerYear);
        return newRatePerBlock;
    }

It just use wallet address as a random seed. You can keep changing the wallet to get the optimal rate.

Tools Used

Manual

Recommended Mitigation Steps

You should reimplement this function

Lines of code

https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L190-L201 https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L154-L171

Vulnerability details

Impact

Attacker may use huge amount of their fund to pump the token in a liquidity pair for one entire block. The oracle will capture the manipulated price as current TWAP implementation may only cover 1 block if timed correctly. (First block on every periodSize = 1800 minutes)

This is possible and similar to Inverse finance April attack: https://www.coindesk.com/tech/2022/04/02/defi-lender-inverse-finance-exploited-for-156-million/

Proof of Concept

Assume currently is the first block after periodSize = 1800 minutes

    function _update(uint balance0, uint balance1, uint _reserve0, uint _reserve1) internal {
        uint blockTimestamp = block.timestamp;
        uint timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
        if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
            reserve0CumulativeLast += _reserve0 * timeElapsed;
            reserve1CumulativeLast += _reserve1 * timeElapsed;
        }

        Observation memory _point = lastObservation();
        timeElapsed = blockTimestamp - _point.timestamp; // compare the last observation with current timestamp, if greater than 30 minutes, record a new event
        if (timeElapsed > periodSize) {
            observations.push(Observation(blockTimestamp, reserve0CumulativeLast, reserve1CumulativeLast));
        }
        reserve0 = balance0;
        reserve1 = balance1;
        blockTimestampLast = blockTimestamp;
        emit Sync(reserve0, reserve1);
    }

timeElapsed > periodSize (Just greater as periodSize is just passed) -> add current cumulative reserve to observations list

Now, let pump the token. And use some technique that inverse finance hacker have used to hold that price for 1 block.

    function current(address tokenIn, uint amountIn) external view returns (uint amountOut) {
        Observation memory _observation = lastObservation();
        (uint reserve0Cumulative, uint reserve1Cumulative,) = currentCumulativePrices();
        if (block.timestamp == _observation.timestamp) {
            _observation = observations[observations.length-2];
        }

        uint timeElapsed = block.timestamp - _observation.timestamp;
        uint _reserve0 = (reserve0Cumulative - _observation.reserve0Cumulative) / timeElapsed;
        uint _reserve1 = (reserve1Cumulative - _observation.reserve1Cumulative) / timeElapsed;
        amountOut = _getAmountOut(amountIn, tokenIn, _reserve0, _reserve1);
    }

1 Block passed

reserve0Cumulative or reserve1Cumulative may now be skyrocketed due to current token pumping.

timeElapsed = block.timestamp - _observation.timestamp is less than 20 seconds (= 1 block) since _observation.timestamp has just stamped in the previous block.

as timeElapsed is less than 20 seconds (= 1 block) this mean _reserve0 and _reserve1 are just a TWAP of less than 20 seconds (= 1 block) which is easily manipulated by Inverse finance pumping attack technique.

As reserve0Cumulative or reserve1Cumulative is skyrocketed in the 1 block timeframe that is being used in TWAP, _reserve0 or _reserve1 also skyrocketed.

As a conclusion, price oracle may be attacked if attacker can pump price for 1 block since TWAP just cover 1 block.

Tools Used

Manual

Recommended Mitigation Steps

You should calculate TWAP average of _reserve0 and _reserve1 in the _update function by using cumulative reserve difference from last update to now which has a duration of periodSize = 1800 minutes.

And when querying for current price you can just return _reserve0 and _reserve1.

Refer to official uniswap v2 TWAP oracle example: https://github.com/Uniswap/v2-periphery/blob/master/contracts/examples/ExampleOracleSimple.sol

Add unchecked to some part of code

Unchecked obviously reduce gas fee in exchange for manual overflow / underflow handler.

https://github.com/Plex-Engineer/stableswap/blob/489d010eb99a0885139b2d5ed5a2d826838cc5f9/contracts/BaseV1-core.sol#L301-L302

        uint amount0In = _balance0 > _reserve0 - amount0Out ? _balance0 - (_reserve0 - amount0Out) : 0;
        uint amount1In = _balance1 > _reserve1 - amount1Out ? _balance1 - (_reserve1 - amount1Out) : 0;

Can be replaced by

unchecked {
        uint amount0In = _balance0 > _reserve0 - amount0Out ? _balance0 - (_reserve0 - amount0Out) : 0;
        uint amount1In = _balance1 > _reserve1 - amount1Out ? _balance1 - (_reserve1 - amount1Out) : 0;
}

since _reserve0 - amount0Out and _reserve1 - amount1Out never negative due to require(amount0Out < _reserve0 && amount1Out < _reserve1, 'IL'); // BaseV1: INSUFFICIENT_LIQUIDITY

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(amount0Out < _reserve0 && amount1Out < _reserve1, 'IL');

Can be replaced with

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

if (amount0Out >= _reserve0 || amount1Out >= _reserve1) revert InsufficientLiquidity();