Attacker can manipulate the requiredQuorum for Different Ballots at once and prevent finalization

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L385-L400 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L317-L339 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L278-L291

Vulnerability details

Impact

The vulnerability presents itself in a way that it is possible for a user to stop important ballots from passing for a period. This is possible because the finalizeBallot function when finalizing ballots, uses the current staked SALT to get the required quorum as a requirement, but does not take into account that between that time it takes for someone to call finalizeBallot the previous amount of Staked SALT that allowed them to reach quorum might have changed, by someone staking a substantial amount of SALT therefore increasing the required quorum they thought they had reached, hence making the proposal unpassable. That lack of check is what exactly will allow the malicious user to acquire and stake a substantial amount of SALT and brick Ballots especially if the Ballot are important and urgent Changes for the Protocol.

Proof of Concept

A Detailed Narrative of a Concept of how this attack can take place is as Follows

• Critical and Important Ballots are proposed(this is because this are the ballots, the attacker could potentially target because of their importance)

3 Ballots are proposed

Whitelisting Ballots - 1 Approval Ballots - 2

These Requires 2 baseBallotQuorum and 3 baseBallotQuorum respectively

• The Users Casts their votes for the ballots and they know they reached quorum based on the amount of SALT they know have being staked
• Ballot duration comes to an end and Voting ends
• The attack window opens for the user they have already acquired the needed tokens to execute this attack

Before the finalize Ballot is called, the attacker stakes a substantial amount of SALT to change the quorum from what the DAO members thought it was to a even higher figure.

To show you the difference we will create a scenario to show the difference before and after the attack

BEFORE ATTACK

Total Amount Of SALT staked = 5,000,0000 Default 10% required Quorum base = 500,000 Quorum for Whitelisting - 2 baseQuorum = 1,000,000 Quorum for Approvals - 3 baseQuorum = 1,500,000

AFTER ATTACK

Attacker stakes SALT = 1,500,000

Total Amount Of SALT staked = 6,500,0000 Default 10% required Quorum base = 650,000 Quorum for Whitelisting - 2 baseQuorum = 1,300,000 Quorum for Approvals - 3 baseQuorum = 1,950,000

Difference of How much the attacker made the quorum short

Whitelisting - 300,000 SALT Approvals - 450,000 SALT

• Now when the finalizeBallot is called the following happens, and they are unable to finalize

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L278C2-L291C4

DAO::finalizeBallot, when called calls Proposals::canFinalizeBallot to check if the Ballot can actually be finalized as shown below

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L385C2-L400C7

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 First two checks will pass but on a closer look at the third check we can see where the vulnerability that allows the attacker to exploit as shown below

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L317C2-L339C4

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

• The most important aspect the function uses the uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT ); to get totalStaked including the attackers newly staked SALT
• Then the quorum for the specific Ballot type is calculated and returned which is already manipulated by the attacker
• Then this check as shown below will fail because the amount of votes the DAO members thought they allocated has already passed quorum

// Check that the required quorum has been reached
        if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
            return false;

Which Means All the ballot that called finalizeBallot After the attackers action will be unable to finalize and get their proposals passed

NOTE: the mathematical example used is just for demonstration purposes, the level of the attack can be much larger than that, it can go as far as the attacker is willing to go.

Tools Used

Manual Review

Recommended Mitigation Steps

A New Staking meant for system should be considered, because it is somehow easy for a determined attacker to brick ballots at will, it all depends on how much of SALT they are willing to stake to achieve their Goal.

Assessed type

DoS

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

This is acceptable as the attacker would have to stake a significant amount of SALT to affect quorum - and then would not be able to unstake it immediately due to the unstaking mechanism.

[Picodes]

Downgrading to Low as although the mechanism is not standard the sponsor's reasoning is convincing.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Hopizi]

Based on your responses, this means we are both on the same page about the quorum being easily manipulated, I will now make some points to address your response

1. Your response about the vulnerability being acceptable, because the amount of SALT used to carry out the attack is too big, is not mostly correct, because the attacker does not need a huge amount to manipulate the quorum, any amount from a few hundred SALT will actually, manipulate the quorum.

Let's consider a situation about what I am talking about, A Situation where the DAO barely meets the required quorum

BEFORE ATTACK Total Amount Of SALT staked = 5,000,000 Default 10% required Quorum base = 500,000 Quorum for Whitelisting - 2 * baseQuorum = 1,000,000

The DAO Meets the required quorum by 1,000,005, Which is by 5

AFTER ATTACK

Attacker stakes SALT = 100

Total Amount Of SALT staked = 5,000,100 Default 10% required Quorum base = 500,010 Quorum for Whitelisting - 2 * baseQuorum = 1,000,020

The difference of How much the attacker made the quorum short Whitelisting - 20 SALT

The 100 SALT is enough to manipulate the ballot and prevent the 1,000,005 SALT Achieved by the DAO from being unable to meet the newly manipulated Quorum

The example I gave in this report is not about the required amount to successfully carry out the attack, but to ensure that the deficit in the manipulated quorum is enough, to make sure the DAO does not reach that quorum like a situation where the DAO beats the required quorum by a decent amount, then more SALT are needed to perform the attack, an attacker may as well sit and track every vote being cast on-chain and calculate the quorum and then the amount of SALT, they would need to Acquire and STAKE to manipulate the required Quorum and DOS The Ballots

2. The impact of these attacks will determine how much the attacker is willing to use to carry out the attack. Example of ballots that attackers have high incentives to attack i. A compromised website URL in a coordinated attack ii. A country that is about to be excluded immediately in a ballot iii. A bad price feed that is about to be removed iv. A malicious token that needs to be removed v. A Proposal to send SALT that is important to the protocol These impacts are actually to mention a few, the DAO controls, everything in this protocol, and having a mechanism that makes it possible to block those decisions is a critical issue for me, also because this attack could be performed on multiple ballots with the same SALT staked, all at once.

The only trusted role is the DAO. There is no other ownership or privileged role as the exchange is decentralized at launch.

3. Another point is about how other DAOs in most protocols operate and how I believe the finalizing should be done, Most DAOs do not Calculate The Quorum directly with the current amount of voting tokens available, as this is susceptible to a large amount of voting tokens being acquired, through flash loans or other means i. At the moment the ballot is opened for voting, a snapshot of the current amount of staked salt should be taken, this way manipulation is not possible in any form, hereby using the current amount of SALT when the ballot was created and calculating the quorum from that prevents the manipulation of the finalization ii. Most protocols utilize this system, to prevent the same manipulation we are talking about, a situation where a malicious user acquires a huge amount of Voting tokens to manipulate the result of the proposal

4. Viewing this attack as the attacker will lose tokens but this is not entirely accurate as The protocol just Locks the attacker's SALT for a Year, to a malicious attacker, which is an acceptable trade-off to perform an attack on critical ballots that will hurt the protocol.

5. Final Points, In the contest page the Attack Ideas section included issues that will affect the DAO

Any issue that would prevent the DAO from functioning correctly.

https://code4rena.com/audits/2024-01-saltyio#top

[Picodes]

@Hopizi thank you for your comment. I still believe what you are describing in your report is an acceptable consequence of the design chosen by the sponsor.

Ballots can't be finalized if the quorum isn't reached and people could still vote. So if someone stakes to increase the quorum, someone else can just vote to match the quorum increase. It only delays the execution of the proposal - which is fine because if the proposal is urgent we assume it will be way above the quorum and this won't be possible - and the upside for the protocol is that more people staked.