c4-bot-9 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L44-L60 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L77-L91 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L162-L177 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L113-L116

Vulnerability details

Impact

The Protocol implements a maximum number of pools that can be whitelisted, which means that if the size of the maximum number is changed and reduced there is a check in the PoolsConfig::whitelistPool that checks that if the current whitelisted pools are actually less than the maximum, but in the function PoolsConfig::changeMaximumWhitelistedPool it doesn't actually effect that check before reducing the number of maximum of tokens, if both are not tracked effectly before the change is made a DOS is possible for at least 10/11 Days as that what it will take for the Parameter change Ballot.

Proof of Concept

A simple and straight forward way the DOS is Possible Will be presented below

The maximum number of whitelisted tokens - 30
Current Number whitelisted pools - 22
New parameter reduction of max tokens by 10 makes it - 20 as shown can be done in the code below

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

function changeMaximumWhitelistedPools(bool increase) external onlyOwner
        {
        if (increase)
            {
            if (maximumWhitelistedPools < 100)
                maximumWhitelistedPools += 10;
            }
        else
            {
            if (maximumWhitelistedPools > 20)
                maximumWhitelistedPools -= 10;
            }

        emit MaximumWhitelistedPoolsChanged(maximumWhitelistedPools);
        }

Now 22 is not less than 20
Which Means That a DOS is on the cards for the PoolsConfig::whitelistPool the next time it is called as shown below

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

function whitelistPool( IPools pools, IERC20 tokenA, IERC20 tokenB ) external onlyOwner
        {
        require( _whitelist.length() < maximumWhitelistedPools, "Maximum number of whitelisted pools already reached" );
//@audit This will not pass the check resulting in a DOS
...more code
                }

The Second DOS comes in form of the Proposals::proposeTokenWhitelisting as shown below

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L162-L177

function proposeTokenWhitelisting( IERC20 token, string calldata tokenIconURL, string calldata description ) external nonReentrant returns (uint256 _ballotID)
        {
        require( address(token) != address(0), "token cannot be address(0)" );
        require( token.totalSupply() < type(uint112).max, "Token supply cannot exceed uint112.max" ); // 5 quadrillion max supply with 18 decimals of precision

        require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );
//@audit This will also not pass, as it is the exact same kind of check as the previous check and it will result in a DOS 
        ...more code
        }

The Check that resulted in the DOS here is the the same as previous when we check the PoolsConfig contract as shown below

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

function numberOfWhitelistedPools() external view returns (uint256)
        {
        return _whitelist.length();
        }

NOTE: Low Probability this happens but High impact as a 10 days DOS happens Stopping any pending Whitelist impossible to complete and also new token proposals ballot wont go through.

Tools Used

Manual Review

Recommended Mitigation Steps

In the changeMaximumWhitelistedPools function a check shold be implemented that the reduction that is about to take place will not allow this kind of miscalculations

Assessed type

DoS

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #991