Canto Dex Oracle contest - Chom's results

Lines of code

https://github.com/code-423n4/2022-09-canto/blob/65fbb8b9de22cf8f8f3d742b38b4be41ee35c468/src/Swap/BaseV1-periphery.sol#L490-L508

Vulnerability details

Impact

Hackers can deploy tokens with respective names as the stable ones to impersonate the stable token. Then hackers can get profit from the malicious price oracle.

Proof of Concept

        string memory symbol = ctoken.symbol();
        if (compareStrings(symbol, "cCANTO")) {
            underlying = address(wcanto);
            return getPriceNote(address(wcanto), false);
        } else {
            underlying = address(ICErc20(address(ctoken)).underlying()); // We are getting the price for a CErc20 lending market
        }
        //set price statically to 1 when the Comptroller is retrieving Price
        if (compareStrings(symbol, "cNOTE")) { // note in terms of note will always be 1 
            return 1e18; // Stable coins supported by the lending market are instantiated by governance and their price will always be 1 note
        } 
        else if (compareStrings(symbol, "cUSDT") && (msg.sender == Comptroller )) {
            uint decimals = erc20(underlying).decimals();
            return 1e18 * 1e18 / (10 ** decimals); //Scale Price as a mantissa to maintain precision in comptroller
        } 
        else if (compareStrings(symbol, "cUSDC") && (msg.sender == Comptroller)) {
            uint decimals = erc20(underlying).decimals();
            return 1e18 * 1e18 / (10 ** decimals); //Scale Price as a mantissa to maintain precision in comptroller
        }

If hackers or malicious admin deploy a non-stable token but has "cNOTE", "cUSDT", or "cUSDC" as symbols, these tokens will act as a stable token while in fact, it isn't.

Recommended Mitigation Steps

Use fixed address whitelisting instead for example if (address(ctoken) == cUSDC_address) ... where cUSDC_address is an immutable variable set on the constructor.

Lines of code

https://github.com/code-423n4/2022-09-canto/blob/65fbb8b9de22cf8f8f3d742b38b4be41ee35c468/src/Swap/BaseV1-periphery.sol#L524-L593 https://github.com/code-423n4/2022-09-canto/blob/65fbb8b9de22cf8f8f3d742b38b4be41ee35c468/src/Swap/BaseV1-core.sol#L45

Vulnerability details

Impact

Using an average of 8 periods, each period covering 30 minutes may cause the price to be extremely delayed. Hackers can profit from these delays.

In crypto, the price can +100% in just 30 minutes. Imagine volatility of 8 * 30 = 240 minutes = 4 hours. Hackers can profit a lot from these delays. Moreover, if the market crash just like UST, the liquidator can't liquidate assets in time due to extremely delayed price, causing bad debt.

Proof of Concept

uint public periodSize = 1800;

Every 30 minutes one observation will be stored.

...
uint price = IBaseV1Pair(pair).quote(address(token), decimals, 8);
...
        for(uint i; i < 8; ++i) {
            uint token0TVL = assetReserves[i] * (prices[i] / decimals);
            uint token1TVL = unitReserves[i]; // price of the unit asset is always 1
            LpPricesCumulative += (token0TVL + token1TVL) * 1e18 / supply[i];
        }
        uint LpPrice = LpPricesCumulative / 8; // take the average of the cumulative prices 
...

Many functions in BaseV1-periphery use an average of 8 periods = 240 minutes = 4 hours which is ridiculously long.

Recommended Mitigation Steps

Reduce periodSize to 5 minutes