c4-bot-5 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L130-L152 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L385-L400 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L162-L178

Vulnerability details

Impact

The protocol implements a system of maxPendingTokensForWhitelisting which limits the number of tokens that can be pending at once, the default proposed number is 5, but it can go as low as 3 and as 12, this means when the Proposals::proposeTokenWhitelisting is called it checks if that the open ballots must be less than the maxPendingTokensForWhitelisting, before a new proposal can be allowed, and a ballot formed for it, the issue is that the way the protocol works is that couple of tokens can be proposed for whitelisting and users will vote on them with their voting power, but after the duration of the ballots, the ballots that did not reach the quorum will not be able to finalize, due to the requirements to finalize the ballots, hence the _openBallotsForTokenWhitelisting will still contain those ballot that did not finalize, and at a point the check inside the Proposals::proposeTokenWhitelisting will be unable to pass, causing a DOS of that function

Proof of Concept

A detailed concept of how the DOS will be achieved will be listed below

Some 4 tokens are proposed and a ballot is opened for them with the same ending date for them, Users Vote for One of The ballot as it is maybe more profitable in a bull market and reaches Quorum
The Other Tokens ballots do not get enough Quorum to pass
DAO::finalizeBallot is called on the first token that has enough quorum to pass after the ballot end time and it finalizes which removes it from pending whitelisting tokens as shown below

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

function markBallotAsFinalized( uint256 ballotID ) external nonReentrant
        {
        require( msg.sender == address(exchangeConfig.dao()), "Only the DAO can mark a ballot as finalized" );

        Ballot storage ballot = ballots[ballotID];

        // Remove finalized whitelist token ballots from the list of open whitelisting proposals
        if ( ballot.ballotType == BallotType.WHITELIST_TOKEN )
            _openBallotsForTokenWhitelisting.remove( ballotID );

        ...more code

As the Time for ballot as passed no one can vote for those ballots that did not reach quorum again, so you try to finalize them to get them removed from the open ballot array so they do not take space, but the issue is what the requirements to finalize ballots are
When DAO::finalizeBallot is called it also calls Proposals::canFinalizeBallot to check if truly the requirements to finalize has been met as shown below

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

function canFinalizeBallot( uint256 ballotID ) external view returns (bool)
        {
        Ballot memory ballot = ballots[ballotID];
        if ( ! ballot.ballotIsLive )
            return false;

        // Check that the minimum duration has passed
        if (block.timestamp < ballot.ballotMinimumEndTime )
            return false;

        // Check that the required quorum has been reached
        if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
            return false;
//@audit Here we Can see Ballot That Does not Reach Quorum for their ballot type will get returned false, rendering finalization impossible

        return true;
        }

That function call will return false based on not meeting the Quorum requirements
This means that They cannot be removed from the _openBallotsForTokenWhitelisting array as the first token was

Then to reach the DOS point we continue

3 More Tokens are proposed just like the first example, same sequence, 1 gets the required quorum the others do not
This means we now have 5 tokens in the _openBallotsForTokenWhitelisting array, this will not pass the proposeTokenWhitelisting check as shown below

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

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 Check Will not pass as the 5 Tokens is Not less than 5 required space
    ...more code

The DOS point is now achieved as New Tokens cant be proposed

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a Different Finalization Mechanism for ending ballots that did not reach quorum especially for token whitelisting ballots, That allows their removal from the _openBallotsForTokenWhitelisting array

Assessed type

DoS

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #991

c4-judge commented 8 months ago

Picodes marked the issue as satisfactory

code-423n4 / 2024-01-salty-findings

DOS of the proposeTokenWhitelisting will happen because of the logic of the finalize ballot #792