Y2k Finance contest - Chom's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L378-L426

Vulnerability details

Impact

Precision error due to division before multiplication.

Proof of Concept

amount.divWadDown(idFinalTVL[id]).mulDivDown(
    idClaimTVL[id],
    1 ether
)

Formula is (amount / idFinalTVL[id]) * idClaimTVL[id]

If amount = 1, idFinalTVL[id] = 1000, idClaimTVL[id] = 1000 result will be (1 / 1000) * 1000 = 0 * 1000 = 0 But if (amount * idClaimTVL[id]) / idFinalTVL[id] result will be (1 * 1000) / 1000 = 1000 / 1000 = 1

Recommended Mitigation Steps

Change formula to (amount * idClaimTVL[id]) / idFinalTVL[id] to multiply before division

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/oracles/PegOracle.sol#L57-L63 https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/oracles/PegOracle.sol#L89-L106

Vulnerability details

Impact

latestRoundData price1 is not checking for stale price. It bypassed the check as it used a low-level call to priceFeed1.latestRoundData instead of calling getOracle1_Price, which has been prevented from returning a stale price.

Proof of Concept

        (
            uint80 roundID1,
            int256 price1,
            uint256 startedAt1,
            uint256 timeStamp1,
            uint80 answeredInRound1
        ) = priceFeed1.latestRoundData();

        int256 price2 = getOracle2_Price();

price1 is using low-level call to latestRoundData in contrast to price2 using calls to getOracle2_Price which has a stale prevention

    /* solhint-disbable-next-line func-name-mixedcase */
    /** @notice Lookup first oracle price
      * @return price Current first oracle price
      */ 
    function getOracle1_Price() public view returns (int256 price) {
        (
            uint80 roundID1,
            int256 price1,
            ,
            uint256 timeStamp1,
            uint80 answeredInRound1
        ) = priceFeed1.latestRoundData();

        require(price1 > 0, "Chainlink price <= 0");
        require(
            answeredInRound1 >= roundID1,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp1 != 0, "Timestamp == 0 !");

        return price1;
    }

    /* solhint-disbable-next-line func-name-mixedcase */
    /** @notice Lookup second oracle price
      * @return price Current second oracle price
      */ 
    function getOracle2_Price() public view returns (int256 price) {
        (
            uint80 roundID2,
            int256 price2,
            ,
            uint256 timeStamp2,
            uint80 answeredInRound2
        ) = priceFeed2.latestRoundData();

        require(price2 > 0, "Chainlink price <= 0");
        require(
            answeredInRound2 >= roundID2,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp2 != 0, "Timestamp == 0 !");

        return price2;
    }

Recommended Mitigation Steps

1. Create an internal function to return all price1 latestRoundData returns and check for stale prices.
2. Rewrite getOracle1_Price to call that internal function and returns just the price.
3. Update latestRoundData to fetch price1 along with other related parameters from the newly created internal function.