c4-bot-10 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/Whitelist.sol#L204-L229

Vulnerability details

Proof of Concept

From the readMe, we can see the normal flow in the case one want's to contribute, https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/README.md#L185, i.e:

Vultisig token will be initially listed on UniswapV3(VULT/ETH pool).
..
In whitelist contract, there's checkWhitelist function which checks if:
If from address is uniswap v3 pool which holds liquidity, then it means, this transfer is the buy action.

Going through the code snippets for the on-chain logic attached to this flow, we can see the checkWhitelist function here https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/Whitelist.sol#L204-L229

    function checkWhitelist(address from, address to, uint256 amount) external onlyVultisig {
        if (from == _pool && to != owner()) {
            // We only add limitations for buy actions via uniswap v3 pool
            // Still need to ignore WL check if it's owner related actions
            if (_locked) {
                revert Locked();
            }
            if (_isBlacklisted[to]) {
                revert Blacklisted();
            }

            if (_allowedWhitelistIndex == 0 || _whitelistIndex[to] > _allowedWhitelistIndex) {
                revert NotWhitelisted();
            }

            // // Calculate rough ETH amount for VULT amount
    //@audit
            uint256 estimatedETHAmount = IOracle(_oracle).peek(amount);
            if (_contributed[to] + estimatedETHAmount > _maxAddressCap) {
                revert MaxAddressCapOverflow();
            }

            _contributed[to] += estimatedETHAmount;
        }
    }

This function is called via the _beforeTokenTransfer hook as clearly documented here and it makes a call to the IOracle(_oracle).peek() whose implementation is here: https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/oracles/uniswap/UniswapV3Oracle.sol#L39-L46

    function peek(uint256 baseAmount) external view returns (uint256) {
        uint32 longestPeriod = OracleLibrary.getOldestObservationSecondsAgo(pool);
        uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod;//@audit
        int24 tick = OracleLibrary.consult(pool, period);
        uint256 quotedWETHAmount = OracleLibrary.getQuoteAtTick(tick, BASE_AMOUNT, baseToken, WETH);
        // Apply 5% slippage
        return (quotedWETHAmount * baseAmount * 95) / 1e20; // 100 / 1e18
    }

From the UniswapV3Oracle contract we can see the value of PERIOD that gets checked against the longestPeriod, and the PERIOD value is 30 minutes, as clearly initialised here: https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/oracles/uniswap/UniswapV3Oracle.sol#L16

Now the peek function is used to compute the twap of a Uniswap V3 pool using data from its oracle, and then it returns the final calculated (quotedWETHAmount * baseAmount * 95) / 1e20 , issue as tagged by the @audit tag here is that in the case where PERIOD < longestPeriod protocol automatically drops the 30 minutes PERIOD and uses the longestPeriod duration that could be very short, keep in mind that as already hinted in this report above and the readMe, at the early stages of protocol we should expect the intialization of the VULT/ETH pool on uniswap, which would mean that the observation could even be as short as 1 block (12 secs), since an attacker can just backrun the intialization with their contribution.

So since the oldest observation for a pool can be a very short timeframe especially when the pool is relatively new, the pool is heavily manipulatable, coupling this with the fact that protocol does not set a low limit as to how much short the observation period can be after querying the oldest observation from the uniswap pool, this then leads to the protocol to allow for ingestions of wrong prices, so a contributor can just back run the initialization by contributing when the pool is new by manipulating the VULT price calculated as (quotedWETHAmount * baseAmount * 95) / 1e20; and inflate their contribution.

Impact

Since the contributed amount is directly gotten from the return data of peek while checking the whitelist _contributed[to] += estimatedETHAmount; in the case where the contribution is via a uniswap by, then the uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod; check allows for heavily manipulated data to be ingested, cause in our case longestPeriod here can be as low as a few blocks, which essentially leads to early tech savvy contributors to inflate the amount of their contributions having the upper hand over other users.

Recommended Mitigation Steps

Consider scrapping the PERIOD < longestPeriod ? PERIOD : longestPeriod; check, or consider having a low boundary, i.e if the longestPeriod <= 5 minutes, revert the contribution and have the contributors wait a bit to ensure the pool is not as easily manipulatable, i.e apply these _(pseudo) _changes:

    function peek(uint256 baseAmount) external view returns (uint256) {
        uint32 longestPeriod = OracleLibrary.getOldestObservationSecondsAgo(pool);
+    require(longestPeriod >= 300, "Too early ");// 300 here is 5 minutes and a place holder, this could be any other duration that's accepted by protocol.
        uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod;//@audit
        int24 tick = OracleLibrary.consult(pool, period);
        uint256 quotedWETHAmount = OracleLibrary.getQuoteAtTick(tick, BASE_AMOUNT, baseToken, WETH);
        // Apply 5% slippage
        return (quotedWETHAmount * baseAmount * 95) / 1e20; // 100 / 1e18
    }

Assessed type

Context

Bauchibred commented 3 months ago

Known and intended,Invalid.

Hi @alex-ppg, the above comment on this issue from the validator is confusing, can't seem to find where this bug case was classified as known during the audit timeline, kindly reassess the finding.

alex-ppg commented 3 months ago

Hey @Bauchibred, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.

This appears to be a duplicate of #15, and the same rationale applies here for the submission being downgraded to QA (L).

This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded. You are free to refute any of my statements factually, however, I strongly implore you to do this with actual code references and examples.

code-423n4 / 2024-06-vultisig-validation