c4-bot-4 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L77-L91

Vulnerability details

Impact

This vulnerability relates to a scenario where the total number of whitelisted pools can temporarily exceed the specified maximum limit. This occurs when proposals to decrease the maximum number of whitelisted pools are approved after proposals to whitelist new tokens have already passed.

Proof of Concept

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L77-L91

Okay, let me explain the concept using a step-by-step scenario; 1.) Initial State: -Let's say the maximumWhitelistedPools is set to 20. -There are already 20 whitelisted pools.

2.) Proposal 1: -Proposes to decrease maximumWhitelistedPools to 10. -The proposal is approved, and maximumWhitelistedPools is updated to 10.

3.) Proposals 2, 3, and 4: -Propose to whitelist new tokens. -All three proposals pass, adding six new whitelisted pools. -The total number of whitelisted pools is now 26, exceeding the new maximumWhitelistedPools of 10.

4.) Proposal 5 and 6: -Propose to increase maximumWhitelistedPools by 10 each. -Both proposals pass, and maximumWhitelistedPools becomes 30.

Exploitation: The contract is now in a state where 26 whitelisted pools exist, even though the maximum limit is set to 30. Now, proposals to whitelist new tokens can be added, as the new maximum allows it

Tools Used

Manual Review

Recommended Mitigation Steps

I'd recommend that the changeMaximumWhitelistedPools function should be modified to check if the current number of whitelisted pools exceeds the new maximum before the change is made. I'd suggest modifying it this way:


{
    uint256 newMaximum = increase ? maximumWhitelistedPools + 10 : maximumWhitelistedPools - 10;
    require(_whitelist.length() <= newMaximum, "Cannot decrease maximum number of whitelisted pools below current count");

    if (increase)
    {
        if (maximumWhitelistedPools < 100)
            maximumWhitelistedPools += 10;
    }
    else
    {
        if (maximumWhitelistedPools > 20)
            maximumWhitelistedPools -= 10;
    }

    emit MaximumWhitelistedPoolsChanged(maximumWhitelistedPools);
}

## Assessed type

Other

c4-judge commented 9 months ago

Picodes changed the severity to QA (Quality Assurance)