DOS of proposals by abusing ballot names without important parameters

Question

DOS of proposals by abusing ballot names without important parameters

c4-bot-3 opened this issue 9 months ago · 8 comments

Lines of code

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

Vulnerability details

Impact

An adversary can prevent legit proposals from being created by using the same ballot name.

Proposals with the same name can't be created, leading to a DOS for some days until the voting phase ends. This can be done repeatedly, after finalizing the previous malicious proposal and creating a new one.

Impacts for each proposal function:

proposeSendSALT(): DOS of all proposals
proposeSetContractAddress(): DOS of specific contract setting by proposing a malicious address
proposeCallContract(): DOS of specific contract call by providing a wrong number
proposeTokenWhitelisting(): DOS of token whitelisting by providing a fake tokenIconURL
All: Prevent the creation of any legit proposal, by providing a fake/malicious description to discourage positive voting

Note: This Impact fits into the Attack Ideas: "Any issue that would prevent the DAO from functioning correctly."

Vulnerability Details

The main issue is that ballots with the same name revert, and the name doesn't contain all the important parameters to create the proposal:

// Make sure that a proposal of the same name is not already open for the ballot
require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );

Proposals.sol#L101-L102

`proposeSendSALT()` Vulnerability Details

Let's see for example the "Send SALT" proposal. It always has the same name sendSALT. Despite this appears to be an expected behaviour, it can be exploited by an adversary.

The minimum ballot duration is 3 days, with a default value of 10 days. Given that ballots can't be finalized before that, an adversary can consistently create malicious proposals to send themselves the SALT token. The proposal will enter the voting period for some days, and when the phase ends, the adversary can finalize it, and immediately create the same proposal.

This will prevent any other legit "Send SALT" proposal from being created.

There are no mechanisms to remove these malicious proposals, or to prevent malicious actors from creating them, nor removing their stake. The cost for the adversary is meaningless, as it requires to execute a tx every few days, and they can still claim rewards from the staked assets needed for the proposals.

`proposeSetContractAddress()` Vulnerability Details

Only the contractName is considered for the ballot name, but not the newAddress.

This means that an attack can be performed to consistently create proposals for a specific contract with a malicious address. This prevents updating the price feeds and the access manager.

`proposeCallContract()` Vulnerability Details

Only the contractName is considered for the ballot name, but not the number with which it will be called.

Same as with the previous attack, an adversary can target a specific contract, and consistently create proposals with a wrong calling number.

`proposeTokenWhitelisting()` Vulnerability Details

The tokenIconURL is missing in the ballot name, so whitelisting proposals can be maliciously created for a specific token with a wrong token icon.

`description()` Vulnerability Details

No proposal includes the description (or its hash) in its ballot name. So an adversary can prevent the creation of the legit proposal, by frontrunning it for example, and change the description to something that users would not vote for.

Proof of Concept

This test for proposeSendSALT() already shows how a new proposal can't be created when there is an existing one. An adversary can exploit that as explained on the Vulnerability Details section. That test could be extended to all the other mentioned functions with their corresponding impacts.

Recommended Mitigation Steps

In order to prevent the DOS, ballot names (or some new id variable) should include ALL the attributes of the proposal: ballotType, address1, number1, string1, and string2. Strings could be hashed, and the whole pack could be hashed as well.

So, if an adversary creates the proposal, it would look exactly the same as the legit one.

In the particular case of proposeSendSALT(), strictly preventing simultaneous proposals as they are right now will lead to the explained DOS. Some other mechanism should be implemented to mitigate risks. One way could be to set a long enough cooldown for each user, so that they can't repeatedly send these type of proposals (take into account unstake time).

Assessed type

Governance

Answer 1 · 2024-02-01T15:56:53.000Z

Picodes marked the issue as duplicate of #620

Answer 2 · 2024-02-01T15:57:09.000Z

Picodes marked the issue as not a duplicate

Answer 3 · 2024-02-01T16:02:22.000Z

Picodes marked the issue as primary issue

Answer 4 · 2024-02-08T12:36:23.000Z

othernet-global (sponsor) confirmed

Answer 5 · 2024-02-18T00:31:44.000Z

ballotNames now include all provided proposal arguments.

othernet-global/salty-io@39921b4

Answer 6 · 2024-02-19T16:37:32.000Z

Picodes marked the issue as satisfactory

Answer 7 · 2024-02-19T17:08:02.000Z

Picodes marked the issue as selected for report

Answer 8 · 2024-02-19T17:11:31.000Z

Flagging as duplicate of #621 all issues about the fact that identifying proposals by names that are not sender-specific and do not include all arguments opens the door to being front-run and could lead to a DoS. Not all reports found all the different cases where this could happen but I gave full credit to the one where the impact was Medium.