c4-bot-8 commented 5 months ago

Lines of code

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

Vulnerability details

Impact

When an ILOPool is initialized with the $VULT token being the sale token, users can buy shares of the liquidity before the project launch (via ILOPool.buy()) then claim their VULT token via ILOPool.claim() where the claimed sale token will be burnt from the pool liquidity and sent to the user.

function claim(uint256 tokenId)
        external
        payable
        override
        isAuthorizedForToken(tokenId)
        returns (uint256 amount0, uint256 amount1)
    {

        //....
                // transfer token to the user
        TransferHelper.safeTransfer(_cachedPoolKey.token0, ownerOf(tokenId), amount0);
        TransferHelper.safeTransfer(_cachedPoolKey.token1, ownerOf(tokenId), amount1);

        //....
    }

The VaultSigWhitelisted token contract has a _beforeTokenTransfer() hook that checks the following before transferring tokens to the recipient:

if the recipient account (to) is whitelisted.
if the recipient contribution is less than the _maxAddressCap, where this is set to 3 ether for each account:

    /// @notice Before token transfer hook
    /// @dev It will call `checkWhitelist` function and if it's succsessful, it will transfer tokens, unless revert
    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal override {
        if (_whitelistContract != address(0)) {
            IWhitelist(_whitelistContract).checkWhitelist(from, to, amount);
        }
        super._beforeTokenTransfer(from, to, amount);
    }

where:

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
            uint256 estimatedETHAmount = IOracle(_oracle).peek(amount);
            if (_contributed[to] + estimatedETHAmount > _maxAddressCap) {
                revert MaxAddressCapOverflow();
            }

            _contributed[to] += estimatedETHAmount;
        }
    }

but this _contributed[to] + estimatedETHAmount < _maxAddressCap check can be bypassed, let's see the following scenario:

an investor buys a large share of the sale token liquidity before project launch via ILOPool.buy() (the max limit to buy for each investor is saleInfo.maxCapPerUser).
when the project is launched; the investor tries to claim his sale tokens ($VULT) via multiple ILOPool.claim() txns over time (as the vesting period ends for each schedule), and when the liquidity is burned from the uniswapv3 pool and transferred to the investor; _beforeTokenTransfer() hook will be invoked by the $VULT whitelisted token contract to check if the investor is whitelisted and if the amount of his claimed tokens doesn't exceed the _maxAddressCap.
when the $VULT is valued at a high price; then the estimatedETHAmount of the $VULT will be high resulting in the total contributions of the investor being > 3 ethers, so his ILOPool.claim() txn will revert with MaxAddressCapOverflow revert message.
the investor can bypass this by transferring his NFT liquidity position to another account that doesn't have any contributions yet, where this new account will be able to claim the sale token without reverting, knowing that this process might be repeated until the full liqudity of the NFT position is calimed (transferring the ownership of the NFT position from one account to another).

note: another two issues were reported regarding the mechanism of the checkWhitelist that have similar introduction but differ in impact: a. Non whitelisted accounts can still have $VULT tokens. b. Investors might not be able to claim their $VULT sale tokens if priced high.

Proof of Concept

VultisigWhitelisted._beforeTokenTransfer()

    function _beforeTokenTransfer(address from, address to, uint256 amount) internal override {
         //...
            IWhitelist(_whitelistContract).checkWhitelist(from, to, amount);
        //...
    }

Whitelist.checkWhitelist()

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
            uint256 estimatedETHAmount = IOracle(_oracle).peek(amount);
            if (_contributed[to] + estimatedETHAmount > _maxAddressCap) {
                revert MaxAddressCapOverflow();
            }

            _contributed[to] += estimatedETHAmount;
        }
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

A mitigation could be preventing transferring position NFT during claiming window, or by introducing a new mechanism to refund investors their raised tokens if their contributions exceeds _maxAddressCap.

Assessed type

Context

DevHals commented 4 months ago

Hi @alex-ppg , This is a duplicate of this issue

alex-ppg commented 3 months ago

Hey @DevHals, 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 submission is indeed a duplicate and will be migrated soon. This issue's discussion thread will be updated with the relevant issue number in the findings repository when that is done.

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.