attacker can DOS admins calls to setPool and prevent protocol from updating

Summary

Admin can call setPool() to set curve MetaPool to be used as a TWAP oracles in the protocol. the issue is that in setPool()'s code checks that for that pool _reserve0 == _reserve1 but attacker can always manipulate and change the reserves in the pool by performing swaps. so attacker can DOS and prevent admins from calling setPool() and admins can't update the curve pool and it may cause wrong TWAP price as it has to work with the old pool while the token of the protocol is updated to new address.

also these lines in setPool() are incorrect too(this can be considered a separate issue):

        ts.price0Average = 1 ether;
        ts.price1Average = 1 ether;

because there is no guarantee that price of the pool would be 1 to 1 always but the require(_reserve0 == _reserve1) bug prevents it. so if required has been removed then the average price should be calculated based on pool's real reserves.

Vulnerability Detail

This is part of the setPool() code, as you can see it checks to make sure that reserve's of the pool for token 0 and 1 is equal:

    function setPool(address _pool, address _curve3CRVToken1) internal {
        require(
            IMetaPool(_pool).coins(0) ==
                LibAppStorage.appStorage().dollarTokenAddress,
            "TWAPOracle: FIRST_COIN_NOT_DOLLAR"
        );
        TWAPOracleStorage storage ts = twapOracleStorage();

        // coin at index 0 is Ubiquity Dollar and index 1 is 3CRV
        require(
            IMetaPool(_pool).coins(1) == _curve3CRVToken1,
            "TWAPOracle: COIN_ORDER_MISMATCH"
        );

        uint256 _reserve0 = uint112(IMetaPool(_pool).balances(0));
        uint256 _reserve1 = uint112(IMetaPool(_pool).balances(1));

        // ensure that there's liquidity in the pair
        require(_reserve0 != 0 && _reserve1 != 0, "TWAPOracle: NO_RESERVES");
        // ensure that pair balance is perfect
        require(_reserve0 == _reserve1, "TWAPOracle: PAIR_UNBALANCED");

but reserves of the pool can be manipulated by attacker by performing swaps in the curve MetaPool and attacker can revert admins calls to setPool() just by performing swaps in the pool (attacker can front run any call to setPool() and change pool's reserves). the assumption that reserves should be equal because they are both stable coin is wrong and exact equality of reserves is not guaranteed in AMM pools even for stable coins. as protocol token is migrated to new address admins need to call setPool() to change the target pool and TWAP prices required correct pool to work. so by preventing update of the pool TWAP price would be wrong and mint and redeem functionality depends on the TWAP price and they will be broken too. This is the POC:

admins wants to set new pool either because of token update or old pool problems.
admins create new pool in the curve protocol and add equal liquidity of both tokens.
attacker would call and make swap in the new pool.
now admins can't call setPool()

setting the price0Average and price1Average to 1 ether is wrong too(wrong assumptions about the pool) but it won't cause problem in the current code because require(reserve0 == reserve1) makes sure that price is 1 to 1, but if require has been removed then the average prices should be calculated correctly. in general this could be a separate issue that will be exploitable if the require(reserve0 == reserve1) has been fixed. setting wrong average price would give attacker opportunity to exploit other logics that depends on the TWAP price(logics that calls consult()). there are multiple logics in the code that calls TWAP price without updating the price first(like _incentivizeBuy() or _incentivizeSell() in LibCurveDollarIncentive, getDollarsToMint() in LibDollarMintCalculator, mintClaimableDollars() in LibCreditNftManager). attacker can call those logics after setPool() transaction, and while the real average price is not 1 ether, code would execute those logics based on 1 ether average price.

Impact

attacker can DOS updating the target metapool for TWAP price and it would break the core functionality of the protocol like mint and redeem.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibTWAPOracle.sol#L50-L51

Tool used

Manual Review

Recommendation

remove the unnecessary check or don't check for exact equality (perform this check: reserves should be in N% of each other)

Duplicate of #14

sherlock-admin2 commented 6 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about DOSing setPool function by manipulating the Curve pool, but it's assumed that the Curve pool deployment, LP deposit, and setPool will be handled in one tx using multicall structure