Salty.IO - cats's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L167 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L88-L95 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAOConfig.sol#L47-L49

Vulnerability details

Impact

If users want to add new whitelisted tokens through the DAO part of the protocol they create proposals/ballots and if enough votes accumulate, the proposal can be finalized and token whitelisted. The max open ballots at the same time for whitelisting a token are capped at amount daoConfig.maxPendingTokensForWhitelisting().

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;
}

The maximum amount of open ballots at a single time is currently capped at 5 and can be increased to up to 12 if the DAO votes for it. Due to this fairly easily reachable limit, an issue exists for DOS-ing the token whitelisting process.

The previous report by Trail of Bits uncovered this vulnerability. It is the first issue in the report at page #17 - TOB-SALTY-1.

https://github.com/trailofbits/publications/blob/master/reviews/2023-10-saltyio-securityreview.pdf

When the contest with C4 started, the sponsor stated that the Trail of Bits recommendations and fixes are already applied, and any issues found on top of the fixed code are valid:

"The issues in the Trail of Bits audit were remediated already. Any issues with the remediation would still be valid of course."

The sponsor also stated the intended fix for this issue:

"The requirement for making proposals has changed from a proposal cost to a minimum amount of staked SALT. Additionally, users are only able to have one active proposal at a time, which combined with xSALT's default one year unstaking period should help to limit spamming of the proposals. To ensure that some adequate total stake exists to be used as a reference, proposals are delayed for the first 45 days after deployment."

Pre-fix, users could create proposals for a flat fee of $500, now there is a minimum of staked SALT required that is a percentage of the total staked SALT. Also, now, users are limited to only 1 active proposal at a time.

// 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" );

But the attack is very well still executable by the same user across 5 different addresses. Even if the minimum staked salt is a percentage of the total supply, it is still a very minimum amount requirement:

// The percent of staked SALT that a user has to have to make a proposal
// Range: 0.10% to 2% with an adjustment of 0.10%

And although the default unstaking period of xSALT is 1 year, the minimum amount is 2 weeks at which the user can redeem 20% of their xSALT so even if this attack is executed, in only 2 weeks the malicious user will still be able to recover 20% of the used xSALT.

Tools Used

Manual review

Recommended Mitigation Steps

I believe the first ToB recommendation makes sense. Implement a centralized entity role which has the ability to remove /only/ open token whitelisting ballots in order to filter them.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L276 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L396 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L319-L329

Vulnerability details

Impact

The voting power used for casting a vote in proposals is derived from the amount of SALT a user has staked (xSALT when staked).

uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );

The quorum that needs to be reached for a proposal to be able to be finalized in Proposals#canFinalizeBallot() is taken from requiredQuorumForBallotType(), it is a percentage of the total amount of STAKED_SALT.

        // 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 );

The issue is that after voting in a proposal, a user can initiate an unstake of all his STAKED_SALT and while his vote stays commited to that Ballot, the needed quorum is now a lower amount due to the function computing the percentage from the total STAKED_SALT. This will heavily sway the ratio of Casted Votes/Quorum Needed for proposals.

Proof of Concept

1. Add the code below to the Proposals.t.sol file.
2. Run with COVERAGE="yes" NETWORK="sep" forge test --match-test testUnstakeQuorumChange -vvvv --rpc-url https://rpc.sepolia.org.

    function testUnstakeQuorumChange() public {
        vm.startPrank(DEPLOYER);
        salt.transfer(alice, 30000000 ether);
        salt.transfer(bob, 30000000 ether);
        salt.transfer(charlie, 30000000 ether);
        
        // Staking SALT as Alice so she is able to create proposal
        vm.startPrank(alice);
        staking.stakeSALT(30000000 ether);

        // Creating proposal as Alice
        proposals.proposeParameterBallot(1, "description" );
        
        // Caching ballotId
        uint256 ballotId = proposals.openBallotsByName("parameter:1");

        // Staking SALT as Bob
        vm.startPrank(bob);
        staking.stakeSALT(30000000 ether);

        // Staking SALT as Charlie and voting with it
        vm.startPrank(charlie);
        salt.approve(address(staking), type(uint256).max);
        staking.stakeSALT(30000000 ether);
        proposals.castVote(ballotId, Vote.INCREASE);

        // Caching votes cast before unstake and required quorum before unstake
        uint256 votesCastBefore = proposals.votesCastForBallot(ballotId, Vote.INCREASE);
        uint256 requiredQuorumBefore = proposals.requiredQuorumForBallotType(BallotType.PARAMETER);

        // Unstaking as Charlie now
        staking.unstake(30000000 ether, 2);

        // Caching votes cast after unstake and required quorum after unstake
        uint256 votesCastAfter = proposals.votesCastForBallot(ballotId, Vote.INCREASE);
        uint256 requiredQuorumAfter = proposals.requiredQuorumForBallotType(BallotType.PARAMETER);

        // This should PASS, votes cast are retained even after unstake
        assertEq(votesCastBefore, votesCastAfter, "Votes Cast Differ");
        // This should FAIL, quorum before unstaking and after unstaking is different even while retaining voting power
        assertEq(requiredQuorumBefore, requiredQuorumAfter, "Required Quorum Differs");
    }

Results

Test should FAIL due to assertEq failing requiredQuorumBefore & requiredQuorumAfter.

Logs:
  Error: Required Quorum Differs
  Error: a == b not satisfied [uint]
        Left: 9000000000000000000000000
       Right: 6000000000000000000000000

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

A mitigation would be to reset the user's vote if they unstake at any given time. That or refactoring the code so that required quorum is not taken from a variable that is subject to change.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L278-L291 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L396 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L319-L329 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L98

Vulnerability details

Impact

In order for a proposal/ballot to be finalized, DAO#finalizeBalot() calls upon Proposals#canFinalizeBallot() and if all checks pass, finalizes the proposal. One of the checks is that the totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType(ballot.ballotType).

	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;
	    }

And how the required quorum for a ballot is calculated is based on the current amount of STAKED_SALT.

        // 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 );

The issue comes from the fact that if a user submits a proposal and it does not reach quorum it will never be able to be finalized, meanwhile there is no mechanism in place to let the user cancel their proposal either, even if the ballotMinimumEndTime has passed. And if a user tries to create a new proposal, they will not be allowed because of this check:

        // 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" );

So if a user has a proposal that does not reach quorum, he can never submit a new proposal from this address again. It is also expected behavior for the amount of STAKED_SALT to increase with time in optimal conditions, which means that the quorum needed will only keep growing.

Proof of Concept

1. Add the code below to the Proposals.t.sol file.
2. Run with COVERAGE="yes" NETWORK="sep" forge test --match-test testQuorumNotReached -vvvv --rpc-url https://rpc.sepolia.org.

    function testQuorumNotReached() public {
        // Staking SALT as Alice so she is able to create proposal
        vm.startPrank(alice);
        staking.stakeSALT(1000e18);

        // Creating proposal with Alice
        proposals.proposeParameterBallot(1, "description" );

        // Caching ballotId
        uint256 ballotId = proposals.openBallotsByName("parameter:1");

        // Casting votes as Alice
        proposals.castVote(ballotId, Vote.INCREASE);

        // Dealing Bob SALT and staking it to simulate natural environment and increase quorum
        deal(address(salt), bob, 4000e18);
        vm.startPrank(bob);
        staking.stakeSALT(4000e18);

        console.log("Total votes cast for ballot:", proposals.totalVotesCastForBallot(ballotId));
        console.log("Required quorum:", proposals.requiredQuorumForBallotType(BallotType.PARAMETER));

        // Attempting to finalize ballot before it reaches quorum votes as Charlie
        vm.startPrank(charlie);
        vm.expectRevert();
        dao.finalizeBallot(ballotId);

        // Attempting to start a new ballot as Alice
        vm.startPrank(alice);
        vm.expectRevert();
        proposals.proposeParameterBallot(2, "description" );
    }

Results

    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [9330] DAO::finalizeBallot(1)
    │   ├─ [3406] Proposals::canFinalizeBallot(1) [staticcall]
    │   │   └─ ← false
    │   └─ ← revert: The ballot is not yet able to be finalized
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111)
    │   └─ ← ()
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [8590] Proposals::proposeParameterBallot(2, "description")
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0x82C87fFA547BA385d1528Edd1f4efFc30F9f1CA9]
    │   ├─ [529] Staking::totalShares(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 5000000000000000000000 [5e21]
    │   ├─ [329] DAOConfig::requiredProposalPercentStakeTimes1000() [staticcall]
    │   │   └─ ← 500
    │   ├─ [701] Staking::userShareForPool(0x0000000000000000000000000000000000001111, 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← 1000000000000000000000 [1e21]
    │   └─ ← revert: Users can only have one active proposal at a time
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 104.01s

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

Allow a user to cancel his ballot if the ballotMinimumEndTime has passed.

Assessed type

Governance