Canto v2 contest - __141345__'s results

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L530-L532 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L206-L214 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L221-L238

Vulnerability details

The stable ctoken underlying price quote could be manipulated

Impact

The underlying price for stable ctoken can be manipulated. When the underlying price is used to in the borrow() or liquidity calculation for the collateral, the malicious user can borrow inproportional amount compare to the real market price.

Proof of Concept

The underlying price is obtained from fucntion getUnderlyingPrice()

file: lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol 530-532:

            stablePair = (stable == 0) ? false : true;
            pair = IBaseV1Pair(pairFor(USDC, underlying, stablePair)); //get the pair for the USDC/underlying pool
            price = pair.quote(underlying, 1, 8); //how much USDC is this token redeemable for

The fucntion quote() is defined as follows, and the result price mainly obtained is from fucntion sample(). file: lending-market-v2/contracts/Stableswap/BaseV1-core.sol 206-214, 221-238:

    function quote(address tokenIn, uint amountIn, uint granularity) external view returns (uint amountOut) {
        console.log("tokenIn: ", tokenIn);
        uint [] memory _prices = sample(tokenIn, amountIn, granularity, 1);
        uint priceAverageCumulative;
        for (uint i = 0; i < _prices.length; i++) {
            priceAverageCumulative += _prices[i];
        }
        return priceAverageCumulative / granularity;
    }

    function sample(address tokenIn, uint amountIn, uint points, uint window) public view returns (uint[] memory) {
        uint[] memory _prices = new uint[](points);

        uint length = observations.length-1;
        uint i = length - (points * window);
        uint nextIndex = 0;
        uint index = 0;

        for (; i < length; i+=window) {
            nextIndex = i + window;
            uint timeElapsed = observations[nextIndex].timestamp - observations[i].timestamp;
            uint _reserve0 = (observations[nextIndex].reserve0Cumulative - observations[i].reserve0Cumulative) / timeElapsed;
            uint _reserve1 = (observations[nextIndex].reserve1Cumulative - observations[i].reserve1Cumulative) / timeElapsed;
            _prices[index] = _getAmountOut(amountIn, tokenIn, _reserve0, _reserve1);
            index = index + 1;
        }   
        return _prices;
    }

The underlying price relies on the quote() function, which call the sample() function with granularity default to 8. Anyone can perform 8 block transaction in the target pair, for the purpose of controling the prcie for the target ctoken.

Another case, if the target pair does not exist, the malicious user can create one, and get any price by design.

Tools Used

mannual analysis

Recommended Mitigation Steps

Use verified oracle, like Chainlink to get the reliable price.

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L522-L526 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L198-L217

Vulnerability details

The LP pair underlying price quote could be manipulated

Impact

The underlying price for LP pool pair can be manipulated. This kind of price mainpulation happened before, can be found here: Warp Fincance event.

Whick may lead to the exploit of the pool by a malicious user.

Proof of Concept

file: lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol 522-526， 198-217:

            uint price0 = (token0 != USDC) ? IBaseV1Pair(pairFor(USDC, token0, stable0)).quote(token0, 1, 8) : 1;
            uint price1 = (token1 != USDC) ? IBaseV1Pair(pairFor(USDC, token1, stable1)).quote(token1, 1, 8) : 1;
            // how much of each asset is 1 LP token redeemable for
            (uint amt0, uint amt1) = quoteRemoveLiquidity(token0, token1, stablePair, 1);
            price = amt0 * price0 + amt1 * price1;


    function quoteRemoveLiquidity(
        address tokenA,
        address tokenB,
        bool stable,
        uint liquidity
    ) public view returns (uint amountA, uint amountB) {
        // create the pair if it doesn"t exist yet
        address _pair = IBaseV1Factory(factory).getPair(tokenA, tokenB, stable);

        if (_pair == address(0)) {
            return (0,0);
        }

        (uint reserveA, uint reserveB) = getReserves(tokenA, tokenB, stable);
        uint _totalSupply = erc20(_pair).totalSupply();

        amountA = liquidity * reserveA / _totalSupply; // using balances ensures pro-rata distribution
        amountB = liquidity * reserveB / _totalSupply; // using balances ensures pro-rata distribution

    }

The price of the LP pair is determined by the TVL of the pool, given by: amt0 * price0 + amt1 * price1. However, when a malicious user dumps large amount of any token into the pool, the whole TVL will be significantly increased, which leads to inproper calculation of the price.

Tools Used

mannual analysis

Recommended Mitigation Steps

A differenct approach to calculate the LP price can be found here.

USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(,)

lending-market-v2/contracts/Governance/GovernorAlpha.sol:256:
bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));


lending-market-v2/contracts/Stableswap/BaseV1-core.sol:110:

        if (_stable) {
            name = string(abi.encodePacked("StableV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
            symbol = string(abi.encodePacked("sAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
        } else {
            name = string(abi.encodePacked("VolatileV1 AMM - ", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
            symbol = string(abi.encodePacked("vAMM-", erc20(_token0).symbol(), "/", erc20(_token1).symbol()));
        }

LARGE MULTIPLES OF TEN SHOULD USE SCIENTIFIC NOTATION (E.G. 1E6) RATHER THAN DECIMAL LITERALS (E.G. 1000000), FOR READABILITY

manifest-v2/contracts\ERC20MaliciousDelayed.sol:11:
uint256 private _bigNum = 1000000000000000000; // ~uint256(0)

CHECK FOR PAIR EXISTANCE IN PERIPHERY

In chained swap, if some pair in the path does not exist, the swap will fail at the end. However, it might be better to add some existance check for the pair early.

lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol

    // performs chained getAmountOut calculations on any number of pairs
    function getAmountsOut(uint amountIn, route[] memory routes) public view returns (uint[] memory amounts) {
        require(routes.length >= 1, "BaseV1Router: INVALID_PATH");
        amounts = new uint[](routes.length+1);
        amounts[0] = amountIn;
        for (uint i = 0; i < routes.length; i++) {
            address pair = pairFor(routes[i].from, routes[i].to, routes[i].stable);
            if (IBaseV1Factory(factory).isPair(pair)) {
                amounts[i+1] = IBaseV1Pair(pair).getAmountOut(amounts[i], routes[i].from);
            }
        }
    }

Also here:

lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol

    function getReserves(address tokenA, address tokenB, bool stable) public view returns (uint reserveA, uint reserveB) {
        (address token0,) = sortTokens(tokenA, tokenB);
        (uint reserve0, uint reserve1,) = IBaseV1Pair(pairFor(tokenA, tokenB, stable)).getReserves();
        (reserveA, reserveB) = tokenA == token0 ? (reserve0, reserve1) : (reserve1, reserve0);
    }

Suggestion: Adding else part and reject the swap early, other than excute more functions.

FILE MISSING NATSPEC

For many files in lending-market-v2

suggestion: Follow NATSPEC.

ADDRESS.CALL{VALUE:X}() SHOULD BE USED INSTEAD OF PAYABLE.TRANSFER()

When operations use a wrapped native token, the withdraw is being handled with a payable.transfer() method.

Using Solidity's transfer function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

• The withdrawer smart contract does not implement a payable fallback function.
• The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
• The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-effects-interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. 

lending-market-v2/contracts/CEther.sol
    function doTransferOut(address payable to, uint amount) virtual override internal {
        /* Send the Ether, with minimal gas and revert on failure */
        to.transfer(amount);
    }

References:

The issues with transfer() are outlined here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles: https://blog.openzeppelin.com/reentrancy-after-istanbul/

Suggestion: Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

MULTIPLE ADDRESS MAPPINGS COMBINED

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT, WHERE APPROPRIATE

lending-market-v2/contracts/Stableswap/BaseV1-core.sol

86:
    // position assigned to each LP to track their current index0 & index1 vs the global position
    mapping(address => uint) public supplyIndex0;
    mapping(address => uint) public supplyIndex1;

MISSING CONTRACT-EXISTENCE CHECKS BEFORE LOW-LEVEL CALLS

Low-level calls return success if there is no code present at the specified address. In addition to the zero-address checks, add a check to verify that <address>.code.length > 0.

The delegateTo function is called in the constructor.

lending-market-v2/contracts/Accountant/AccountantDelegator.sol


    constructor(
            address implementation_,
            address admin_,
            address cnoteAddress_,
            address noteAddress_,
            address comptrollerAddress_,
            address treasury_) {
        require(admin_ != address(0));
        // Admin set to msg.sender for initialization
        admin = msg.sender;
        delegateTo(implementation_, abi.encodeWithSignature("initialize(address,address,address,address)",
                                                            treasury_,
                                                            cnoteAddress_,
                                                            noteAddress_,
                                                            comptrollerAddress_));
        setImplementation(implementation_);
        admin = admin_;
    }



80:
function delegateTo(address callee, bytes memory data) internal returns(bytes memory) {
        (bool success, bytes memory returnData) = callee.delegatecall(data);

125:
(bool success, ) = implementation.delegatecall(msg.data);

Same for

lending-market-v2/contracts/Governance/GovernorBravoDelegator.sol

lending-market-v2/contracts/Treasury/TreasuryDelegator.sol