Whitelisted Addresses Can Become Non-Whitelisted by Lowering `_allowedWhitelistIndex`

[c4-bot-1]

Lines of code

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

Vulnerability details

Proof of Concept

In the Whitelist contract, the _allowedWhitelistIndex variable determines the maximum index up to which addresses are considered whitelisted. The checkWhitelist function enforces this by checking if an address's _whitelistIndex is within the allowed range before allowing certain actions, such as token transfers.

Here is the relevant portion of the checkWhitelist function:

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;
    }
}

If the admin lowers the _allowedWhitelistIndex, some addresses that were previously considered whitelisted (i.e., their _whitelistIndex was less than or equal to the previous _allowedWhitelistIndex) will no longer be considered whitelisted if their _whitelistIndex exceeds the new _allowedWhitelistIndex.

For example, if the _allowedWhitelistIndex is reduced from 100 to 50, addresses with a _whitelistIndex between 51 and 100 will be excluded from the whitelist, even though they were previously included.

Impact

Lowering the _allowedWhitelistIndex can cause legitimate, previously whitelisted users to lose their whitelisted status.

Tools Used

Manual

Recommended Mitigation Steps

One possible mitigation is to include a function that allows updating the _whitelistIndex values for addresses when _allowedWhitelistIndex is lowered:

function updateWhitelistIndexes(address[] calldata addresses, uint256 newIndex) external onlyOwner {
    require(newIndex <= _allowedWhitelistIndex, "New index must be within allowed range");
    for (uint i = 0; i < addresses.length; i++) {
        if (_whitelistIndex[addresses[i]] > _allowedWhitelistIndex) {
            _whitelistIndex[addresses[i]] = newIndex;
        }
    }
}

Assessed type

Context

[Odhiambo526]

@alex-ppg May I kindly request for why this issue was marked invalid. I am yearning to know what I did miss! Thank you.

[alex-ppg]

Hey @Odhiambo526, 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.

Per the documentation of the system, lowering the index will deliberately make a whitelisted address not be whitelisted anymore (i.e. act as a blacklist). As such, the vulnerability described is actually an intended feature.

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.