Adversary can prevent updating price feed addresses by creating poisonous proposals ending in `_confirm`

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

An adversary can prevent the creation of setContract and websiteUpdate proposals by creating a poisonous proposal with the same ballot name + _confirm. This attack can be performed repeatedly every few days (when the voting period ends).

Affected proposals:

• setContract:priceFeed1 -> setContract:priceFeed1_confirm
• setContract:priceFeed2 -> setContract:priceFeed2_confirm
• setContract:priceFeed3 -> setContract:priceFeed3_confirm
• setContract:accessManager -> setContract:accessManager_confirm
• setURL:{{newWebsiteURL}} -> setURL:{{newWebsiteURL}}_confirm

This is especially worrisome for the setContract proposals, as it prevents changing the price feed contracts, which are used for USDS borrowing and liquidations.

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

Vulnerability Details

setContract and websiteUpdate proposals are executed in multiple steps.

Here's the process for changing the Price Feed 1, as an example:

1. A new proposal is created with a ballot name setContract:priceFeed1
2. The proposal is voted, and when it wins, a confirmation proposal is created, appending _confirm to the ballot name, resulting in setContract:priceFeed1_confirm.
3. When the confirmation proposal wins, it checks its ballot name and then it sets the new price feed.

The problem is that there is a check in _possiblyCreateProposal() that can be exploited:

require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

Proposals.sol#L103

This check is intended to prevent creating new proposals if there is a pending confirmation proposal being voted for the same change, but an adversary can use it to create a proposal with a ballot name setContract:priceFeed1_confirm, by setting priceFeed1_confirm as the contract name.

If someone tries to create a legit priceFeed1 proposal later, it will revert, leading to a DOS.

This holds for all proposals mentioned in the Impact section.

Proof of Concept

1. Add the test to src/dao/tests/Proposals.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://1rpc.io/sepolia --mt "testProposeSetContractAddress_confirm"

function testProposeSetContractAddress_confirm() public {
    uint256 initialNextBallotId = proposals.nextBallotID();

    // Alice is the adversary
    vm.startPrank(alice);
    staking.stakeSALT(1000 ether);

    // Create a poisonous proposal with a ballot name ending in `_confirm`
    address newAddress = address(0x1111111111111111111111111111111111111112);
    proposals.proposeSetContractAddress("priceFeed1_confirm", newAddress, "description");
    assertEq(proposals.nextBallotID(), initialNextBallotId + 1); // proposal was created successfully
    vm.stopPrank();

    // Transfer some tokens to Bob who wants to create a legit proposal
    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 1000 ether);
    vm.stopPrank();

    // Bob can't create a legit proposal to change the contract address of `priceFeed1`
    vm.startPrank(bob);
    staking.stakeSALT(1000 ether);
    vm.expectRevert("Cannot create a proposal for a ballot with a secondary confirmation");
    proposals.proposeSetContractAddress("priceFeed1", newAddress, "description");
}

Recommended Mitigation Steps

Given the current implementation, the most straightforward fix would be to prevent the creation of proposals with ballot names ending in _confirm for proposals that need a confirmation.

This would mean checking the contractName in proposeSetContractAddress(), and the newWebsiteURL in proposeWebsiteUpdate().

But, as a recommendation, I would suggest refactoring createConfirmationProposal() to pass a "confirmation" parameter to _possiblyCreateProposal(), so that confirmation proposals don't rely on the ballot name.

Assessed type

Governance

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

confirm_ is now prepended to automatic confirmation ballots form setWebsiteURL and setContract proposals.

https://github.com/othernet-global/salty-io/commit/5aa1bc1ddadd67cd875de932633948af25ff8957

[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 selected for report

[J4X-98]

Hi @Picodes,

this issue is also a duplicate of #956 / #980 and exploits the exactly same attack path of appending "_confirm" to the name.

[nonseodion]

Hi @Picodes, thanks for the judging so far.

I am just curious about why this issue is currently of Medium risk. Considering it completely prevents the DAO from setting new price feed and access manager contracts. I'd argue it should be high because of those reasons.

[othernet-global]

The stablecoin framework: /stablecoin, /price_feed, WBTC/WETH collateral, PriceAggregator, price feeds and USDS have been removed: https://github.com/othernet-global/salty-io/commit/88b7fd1f3f5e037a155424a85275efd79f3e9bf9

[nonseodion]

I don't think the severity of a finding depends on changes made after the contest. It depends on how it affects the codebase at the time of the contest.

[Picodes]

@nonseodion isn't it clear from the definitions of severities under C4?

"2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements. 3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals)."

[Picodes]

Here assets are not at direct risk but a "function of the protocol or its availability could be impacted"

[Picodes]

@J4X-98 sorry I am not sure to understand, the findings you are referring to are already flagged as duplicates?

[J4X-98]

@Pico, sorry that was a mistake on my side. I only saw that both were still open and missed the duplicate tag. Sry for the inconvenience.

[Picodes]

No worries! Thanks for flagging in case it was a mistake