Salty.IO - inzinko's results

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

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

[L-01] Users can still vote after the ballot expiry is over

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

If the Ballot end time has been completed, users are not supposed to be able to cast vote again, but the cast vote does not check that the ballot end time has reached

function castVote( uint256 ballotID, Vote vote ) external nonReentrant
		{
		Ballot memory ballot = ballots[ballotID];

		// Require that the ballot is actually live
		require( ballot.ballotIsLive, "The specified ballot is not open for voting" );

//@audit This check here that asks if the ballot is live or not does not prove that the ballot end time has been completed, because that value is only updated if the ballot has been finalized

...more code
		}

• Only on finalizeBallot does ballotIsLive gets updated This means that before finalize ballot is called users can still cast their vote

Mitigation Implement a check to see if The ballot duration has ended

[L-02] Different amount of SALT rewards from Protocol Owned Liquidity to be burnt from the Docs and the Code Implementation

In the Docs of the project, it says that 45% of SALT rewards must be burnt, but this value is different in the code

the docs as shown below

It generates yield for the DAO in the form of SALT. By default 45% of this generated SALT is burned.

https://docs.salty.io/decentralization/protocol-owned-liquidity

But in the code of the DAOConfig The Value is 50%

as shown below

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAOConfig.sol#L26C2-L28C49

// For rewards distributed to the DAO, the percentage of SALT that is burned with the remaining staying in the DAO for later use.
	// Range: 25% to 75% with an adjustment of 5%
    uint256 public percentPolRewardsBurned = 50;

[L-03] Pools contract can contain Unwhitelisted Tokens

In the Pools::deposit users can deposit unwhitelisted tokens because the function does not check if the tokens that are about to be sent are whitelisted tokens, it just allow them to deposit them as shown below

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L205C2-L215C4

function deposit( IERC20 token, uint256 amount ) external nonReentrant
		{
        require( amount > PoolUtils.DUST, "Deposit amount too small");

		_userDeposits[msg.sender][token] += amount;

		// Transfer the tokens from the sender - only tokens without fees should be whitelsited on the DEX
		token.safeTransferFrom(msg.sender, address(this), amount );
//@audit No check for Non-Whitelisted token 

		emit TokenDeposit(msg.sender, token, amount);
		}

This Goes against the rules of the protocol, as only whitelisted tokens should be present in the Pool and any other contract