search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

Minimum quorum can be bypassed by burning salt #301

Closed c4-bot-1 closed 7 months ago

c4-bot-1 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L335 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L114-L120

Vulnerability details

Bug Description

In the Salty DAO's voting system, the requiredQuorumForBallotType() function is pivotal in ensuring a ballot receives votes from a sufficient percentage of stakers. This function calculates a required quorum based on the total stakes and compares it to a minimumQuorum, set as 0.5% of the total SALT token supply. If the calculated quorum is below this minimum, the minimumQuorum is used instead.

function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
    {
    // The quorum will be specified as a percentage of the total amount of SALT staked
    uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
    require( totalStaked != 0, "SALT staked cannot be zero to determine quorum" );

    if ( ballotType == BallotType.PARAMETER )
        requiredQuorum = ( 1 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
    else if ( ( ballotType == BallotType.WHITELIST_TOKEN ) || ( ballotType == BallotType.UNWHITELIST_TOKEN ) )
        requiredQuorum = ( 2 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
    else
        // All other ballot types require 3x multiple of the baseQuorum
        requiredQuorum = ( 3 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );

    // Make sure that the requiredQuorum is at least 0.50% of the total SALT supply.
    // Circulating supply after the first 45 days of emissions will be about 3 million - so this would require about 16% of the circulating
    // SALT to be staked and voting to pass a proposal (including whitelisting) 45 days after deployment..
    uint256 totalSupply = ERC20(address(exchangeConfig.salt())).totalSupply();
    uint256 minimumQuorum = totalSupply * 5 / 1000;

    if ( requiredQuorum < minimumQuorum )
        requiredQuorum = minimumQuorum;
    }

However, a vulnerability exists due to the fluctuating totalSupply of SALT, which changes when tokens are burned. A user close to the 0.5% ownership threshold can vote, then unstake and burn their SALT, effectively lowering the minimum quorum threshold. This allows the user to finalize a proposal even though they voted with less than 0.5% of the total supply.

Impact

This issue enables a malicious staker to finalize a ballot below the intended minimum quorum, potentially manipulating the outcome of key protocol decisions.

Proof of Concept

A test scenario demonstrates how this issue can be exploited:

function testQuorumReducedByBurning() public {
    //------------------------ SETUP ------------------------

    // Set minUnstakeWeeks to 1 which is in the allowed range
    vm.store(address(stakingConfig), bytes32(uint256(1)), bytes32(uint256(1)));

    // Set minUnstakePercent to 10% which is in the allowed range
    vm.store(address(stakingConfig), bytes32(uint256(3)), bytes32(uint256(10)));

    // Set up the parameters for the proposal
    uint256 proposalNum = 0; // Assuming an enumeration starting at 0 for parameter proposals
    address proposer = alice;

    //------------------------ ISSUE ------------------------

    // Alice stakes her SALT to get voting power
    vm.prank(address(daoVestingWallet));
    salt.transfer(proposer, 1000000 ether);

    vm.startPrank(proposer);
    staking.stakeSALT(499999 ether); // CLosely below the minimum quorum

    // Propose a parameter ballot
    proposals.proposeParameterBallot(proposalNum, "Increase max pools count");
    vm.stopPrank();

    uint256 ballotID = 1;

    // Cast a vote, but not enough for quorum
    vm.startPrank(proposer);
    proposals.castVote(ballotID, Vote.NO_CHANGE);
    vm.stopPrank();

    // Increase block time so the ballot would finilaizable if the quorum is passed
    vm.warp(block.timestamp + daoConfig.ballotMinimumDuration());

    // Expect revert because minimum quorum is still not reached
    vm.expectRevert("The ballot is not yet able to be finalized");
    dao.finalizeBallot(ballotID);

    //Nor burn the SALT by unstaking
    vm.startPrank(proposer);
    uint256 unstakeId = staking.unstake(499999 ether - 1, 1);

    // Make a week pass (the unstaking could have also been taking place directly when unstaking)
    vm.warp(block.timestamp + 1 weeks);

    // Finish the unstake (burning 90% of the SALT for the ) falling below the quorum
    staking.recoverSALT(unstakeId);

    // Now it should be possible to finalize the ballot without reverting
    dao.finalizeBallot(ballotID);

    // Check that the ballot is finalized
    bool isBallotFinalized = proposals.ballotForID(ballotID).ballotIsLive;
    assertEq(isBallotFinalized, false, "Ballot should be finalized");
}

The test must be added to /dao/tests/Dao.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testQuorumReducedByBurning".

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, the Ballot struct should include the totalSupply of SALT at the time of ballot creation. This value should then be used to calculate the minimum quorum when finalizing the ballot:

struct Ballot
    {
    uint256 ballotID;
    bool ballotIsLive;

    BallotType ballotType;
    string ballotName;
    address address1;
    uint256 number1;
    string string1;
    string description;
    uint256 totalSupply; <---- Added

    // The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance.
    uint256 ballotMinimumEndTime;
    }

Incorporating the totalSupply into the Ballot struct ensures that the minimum quorum reflects the SALT supply at the time of voting, preserving the integrity of the voting process and preventing manipulation through token burning.

Assessed type

Governance

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #46

c4-judge commented 7 months ago

Picodes marked the issue as satisfactory

J4X-98 commented 6 months ago

Hey @Picodes,

thanks for reviewing this submission. This issue is not a duplicate of #46. I have already submitted the vulnerability, which is described in #46 in my separate issue #187.

The 2 described vulnerabilities look similar, but target different issues and also require different fixes.

This issue:

Issue #46:

InfectedIsm commented 6 months ago

Hi, the fact that the user has to burn its SALT (i.e lose money) only to be able to propose pass a proposal, plus the fact that this can only happens when less that 0.5% SALT are staked make it very unlikely to happen.

716 is considered medium when user isn't incurring any loss, and the exploit can be achieved regardless of the quantity of staked SALT.

So I hardly see how this submission can be considered a medium risk too.

J4X-98 commented 6 months ago

Hi @InfectedIsm,

First I would like to add that you seem to misunderstand the issue. The minimumQuorum is enforced when passing ballots, not when proposing them. Additionally I would like to answer your added points:

the fact that the user has to burn its SALT (i.e lose money) only to be able to propose a proposal, ..., make it very unlikely to happen

In a Med Severity issue, there does not needs to be any profit and there can be a loss for the exploiter. The requirement, as per the Severity Classification is "Assets not at direct risk, but the function of the protocol or its availability could be impacted". In this case the intended functionality of the minQuorum is clearly impacted. If it is unlikely that an attacker abuses it or not does not impact the severity. If permanently loosing salt to make a ballot pass makes sense will depend on the ballot. If a user can gain something worth more than the salt lost from the ballot being passed, it can be a reasonable call.

the fact that this can only happens when less that 0.5% SALT are staked make it very unlikely to happen

The restriction of it only being possible while less than 0.5% of salt are staked does not affect the validity of the issue in any way, it just restricts in which protocol states it can happen. As 0.5% of salt being staked is a valid protocol state, this does not change the issue.

J4X-98 commented 6 months ago

But lets wait for @Picodes judging :)

InfectedIsm commented 6 months ago

Hi @J4X-98, my bad for having mixed "propose" and "pass" when expressing myself as this is obviously worse to be able to pass a proposal than to be only able propose it. That was clearly "pass" that I had in mind, so correcting it.

I'm definitely not saying that this finding is invalid, but rather expressing the fact that likelihood of being executed is far away from #716 and wanted to highlight it.

"If permanently loosing salt to make a ballot pass makes sense will depend on the ballot. If a user can gain something worth more than the salt lost from the ballot being passed, it can be a reasonable call." I totally agree, and no scenario has been demonstrated here on how this could be exploited to gain an advantage, which seems pretty difficult by the fact that proposals are very limited on Salty, as parameters can only be modified by ~10% steps of the value.

But I completely see your point here, I think I made mine and as you said will leave this to judge now

Picodes commented 6 months ago

@J4X-98 to me it's the same issue and the proof is that your mitigations are in fact the same suggestion: store the quorum during the proposal creation to avoid any manipulation during the voting phase. Let me know if I am missing something.

J4X-98 commented 6 months ago

Hi @Picodes,

thanks for reviewing my comment. In my opinion, the 2 are different issues, although they are fixed similarly. They result from different root issues, break different functionalities in the code, and are exploited in different ways.

Attack on stakedSalt

Root Issue: Missing storage of stakedSalt variable in the ballot struct Broken Functionality: requiredQurorum gets set lower than intended allowing to pass votes when a lot of people have staked Attack: Vote->Unstake Salt->Pass Ballot->CancelUnstake Fix: Save the stakedSalt parameter in the ballot struct

Attack on totalSalt

Root Issue: Missing storage of totalSalt variable in the ballot struct Broken Functionality: minimumQuorum will not overwrite the requiredQuorum as the following snippet does not get executed:

    if ( requiredQuorum < minimumQuorum )
        requiredQuorum = minimumQuorum;
    }

This will allow ballots to pass easier than intended if very few have staked which should result in triggering the overwrite of requiredQuorum with minimumQuorum.

Attack: Vote->Unstake Salt at worst possible conditions -> Wait 1 week -> Finish Unstake and burn Salt -> Pass Vote Fix: Save the totalSalt parameter in the ballot struct

Argumentation:

This differentiates the issues in my opinion. From my knowledge of the C4 judging guidelines to be duplicated, issues have to result from the same root issue, which is not the case here.

Additionally, I would like to add that even if the issues were bundled into a root, then all issues that only found the incorrect use of stakedSalt would be 50% dups of the main issue, and this issue and #187 would together make up the main issue describing the full impact of the vulnerability. This is especially the case as the current primary issue does not in any way describe the problem of totalSalt being decreased by burning, or mentions a mitigation for it. Which could result in a sponsor reading the report, potentially only fixing 1 of the 2 issues.

Picodes commented 6 months ago

@J4X-98 I still respectfully disagree.

To me, the root issue is clearly that the quorum isn't stored. Note how the mitigation of https://github.com/code-423n4/2024-01-salty-findings/issues/716 solves both issues. Then, as the quorum depends on 2 variables (the staked SALT and the total supply) you very smartly came with 2 scenarios. But that doesn't make it 2 distinct issues in my opinion.

Regarding how to handle duplicate findings and partial credits, the rule is:

"All issues which identify the same functional vulnerability will be considered duplicates regardless of effective rationalization of severity or exploit path. However, any submissions which do not identify or effectively rationalize the top identified severity case may be judged as “partial credit” and may have their shares divided at judge’s sole discretion."

So here all issues identifying the fact that you can manipulate the quorum by unstaking should get full credit as it is "the top identified severity case".