Users can improperly boost their vote for a proposal to manipulate quorum check

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L60-L79

Vulnerability details

Impact

A user is able to cast a number of votes per proposal equal to the amount of staked SALT they have. Additionally, there needs to be a total amount voted (irrespective of the actual vote), for a proposal to pass the required quorum. For the quorum check, when validating whether a vote can be passed, they check whether the total number of votes cast (in favor of any action) divided by the current total SALT staked is > some threshold. An attacker can abuse the current logic by first casting a vote for a proposal, then unstake. When unstaking, the previous vote from the user will not be removed, meaning that the user is able to in effect both increase the numerator AND decrease the denominator, which will allow them to effectively amplify their vote to pass the quorum check. Additionally, once the vote passes, the user can cancel their unstaking, returning them to their starting state.

Proof of Concept

Let's assume that there is a proposal to send SALT to an address. Let's say there is a total of 100_000e18 staked SALT, and an malicious user has 10_000e18 SALT, with the required quorum for this vote being 30% of all staked SALT. Additionally there is 17_001e18 SALT voted in favor of this proposal already. If the malicious user had just voted for this proposal normally, the total votes would have ended up being 17_001e18+10_000e18=27_001e18 SALT, which is less than 0.3 * 100_000e18=30_000e18 quorum check, meaning the vote wouldn't pass.

However, the malicious user can perform the following attack to boost their vote. They first call Proposals:castVote to vote for the proposal, which has the following logic to increase the vote count of this proposal:

function castVote( uint256 ballotID, Vote vote ) external nonReentrant
    {
    ...
    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);
    }

They then call Unstaking:unstake to unstake their entire SALT balance, which has no reference to the user's previous votes, and also has the following snippet of logic:

_decreaseUserShare( msg.sender, PoolUtils.STAKED_SALT, amountUnstaked, false );

The StakingRewards:_decreaseUserShare call has the following logic, which as expected decreases the pool's totalShares:

totalShares[poolID] -= decreaseShareAmount;
user.userShare -= uint128(decreaseShareAmount);

Now, let's see how these actions impact the logic for finalizing a ballot, which in part is based on the following check in Proposals:requiredQuorumForBallotType:

if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
    return false;

Here, totalVotesCastForBallot(ballotID) will return 27_001e18. The call to Proposals:requiredQuorumForBallotType has the following logic:

uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
...
requiredQuorum = ( 3 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
...

As shown earlier, with the malicious user unstaking, now totalStaked will be 90_000e18. Since 27_001e18 > 0.3 * 90_000e18=27_000e18, the earlier check will pass, meaning the user has successfully circumvented the quorum check. They can then simply call Staking:cancelUnstake to get back to their starting state.

Tools Used

Manual review

Recommended Mitigation Steps

When a user unstakes SALT, all votes they cast should be removed.

Assessed type

Governance

[c4-judge]

Picodes marked the issue as duplicate of #46

[c4-judge]

Picodes marked the issue as satisfactory