Salty.IO - 0xWaitress's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187

Vulnerability details

Impact

last liquidity staker fails to remove their liquidity due to DUST checks in removeLiquidity in Pool.

function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
{
...
require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
}

Proof of Concept

NA

Tools Used

Recommended Mitigation Steps

Consider to remove this check if the reserve0 and reserve1 are both 0 after deducing the liquidity to unstake.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L320-L334

Vulnerability details

Impact

quorum of a proposal would change base on the amount of staked salt after the proposal went live. Consider a ballot type has a 10% quorum. Given a totalStake of 1m SALT in the beginning:

1. proposal A is created.
2. 100k SALT staker voted, the proposal has more Ys than No.
3. After minimum time has passed, the proposer (or anyone) can call finalize to finalize the ballot. However at this point a vetoer, or a malicious actor, stake some more SALT, say 10k, to increase the total staked SALT to 1.01m, increasing the quorum to 101k. More vote is needed.
4. The finalizeBallot tx revert

Impact: non-deterministic finalizeBallot action, since the quorum becomes a floating variable based on the amount of stake SALT. If oppositely there is a major unstaking happening, it would also lead to an unexpected reduction of quorum to the proposal.

	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;

        return true;
	    }

whereas the requiredQuorumForBallotType

	function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
		{
		// The quorum will be specified as a percentage of the total amount of SALT staked
		uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
		require( totalStaked != 0, "SALT staked cannot be zero to determine quorum" );

		if ( ballotType == BallotType.PARAMETER )
			requiredQuorum = ( 1 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
		else if ( ( ballotType == BallotType.WHITELIST_TOKEN ) || ( ballotType == BallotType.UNWHITELIST_TOKEN ) )
			requiredQuorum = ( 2 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
		else
			// All other ballot types require 3x multiple of the baseQuorum
			requiredQuorum = ( 3 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );

		// Make sure that the requiredQuorum is at least 0.50% of the total SALT supply.
		// Circulating supply after the first 45 days of emissions will be about 3 million - so this would require about 16% of the circulating
		// SALT to be staked and voting to pass a proposal (including whitelisting) 45 days after deployment..
		uint256 totalSupply = ERC20(address(exchangeConfig.salt())).totalSupply();
		uint256 minimumQuorum = totalSupply * 5 / 1000;

		if ( requiredQuorum < minimumQuorum )
			requiredQuorum = minimumQuorum;
		}

Proof of Concept

NA

Tools Used

Recommended Mitigation Steps

1. Consider to snapshot the quorum (absolute amount of SALT) at the time proposal creation.

Assessed type

Payable

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L102

Vulnerability details

Impact

creating proposal through _possiblyCreateProposal uses a string as the key, which can be frontrun in Ddos.

all proposal creation calls _possiblyCreateProsposal and returns a ballotId when sucessful. Since the proposal is logged by using a string as a key, if the key/string gets created by another proposal, then the ballot creation would fail. Malicious attackers can make use of this and frontrun any honest proposer with a proposal of the same name, but differnt content in terms of parameters or ballot type.

While the attaker is required to have staked Salt to propose, they can repeatedly ddos proposer once they have the required amount of staked tokens.

	function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
		{
		require( block.timestamp >= firstPossibleProposalTimestamp, "Cannot propose ballots within the first 45 days of deployment" );

		// The DAO can create confirmation proposals which won't have the below requirements
		if ( msg.sender != address(exchangeConfig.dao() ) )
			{
			// Make sure that the sender has the minimum amount of xSALT required to make the proposal
			uint256 totalStaked = staking.totalShares(PoolUtils.STAKED_SALT);
			uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );

			require( requiredXSalt > 0, "requiredXSalt cannot be zero" );

			uint256 userXSalt = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
			require( userXSalt >= requiredXSalt, "Sender does not have enough xSALT to make the proposal" );

			// Make sure that the user doesn't already have an active proposal
			require( ! _userHasActiveProposal[msg.sender], "Users can only have one active proposal at a time" );
			}

		// Make sure that a proposal of the same name is not already open for the ballot
		require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
		require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

		uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();

		// Add the new Ballot to storage
		ballotID = nextBallotID++;
		ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
		openBallotsByName[ballotName] = ballotID;

Proof of Concept

NA

Tools Used

Recommended Mitigation Steps

Consider :

1. removing the use of ballot name as the check for duplication.
2. consider using a hash of the entire input (ballot type, ballot name, description and other argument) to do duplication check

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L168

Vulnerability details

Impact

maximiumWhitelistedPool can be exceeded in proposeTokenWhitelisting

proposeTokenWhitelisting, there are two caps, the maxPendingTokensForWhitelisting and maximumWhitelistedPools. However when stakers propose a token to be whitelisted, these two parameters are checked individually, but not checked against as one total sum.

1. Assume the maxPendingTokensForWhitelisting is 3; and maximumWhitelistedPools is 10.
2. At a given point, there are 8 whitelistenToken. 0 pending.
3. Now there are 3 person, A, B,C each them proposed to whitelist a token, filling up the pending list. Since there are only 8 whitelistedToken so all of their proposal is successfully created.
4. All three tokens proposal are passed and whitelisted, making the actual whitelisted tokens exceeding the maximum cap.

	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" );
		require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );
		require( ! poolsConfig.tokenHasBeenWhitelisted(token, exchangeConfig.wbtc(), exchangeConfig.weth()), "The token has already been whitelisted" );

		string memory ballotName = string.concat("whitelist:", Strings.toHexString(address(token)) );

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

		return ballotID;
		}

Proof of Concept

NA

Tools Used

Recommended Mitigation Steps

Consider constraining the proposal of token whitelist by the sum of pending and whitelisted.

		require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );
---		require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );
+++ require( poolsConfig.numberOfWhitelistedPools() + _openBallotsForTokenWhitelisting.length() < poolsConfig.maximumWhitelistedPools()

Assessed type

Invalid Validation