Attackers can brick proposals by creating fake confirmation proposals

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L81

Vulnerability details

Impact

Malicious users can brick proposals like SET_CONTRACT and SET_WEBSITE_URL by creating another proposal that end with "_confirm".

Proof of Concept

When finalizing approval proposals, the following function from the DAO.sol contract is called:

function _executeApproval( Ballot memory ballot ) internal
        {
//unrelated functionality
// Once an initial setContract proposal passes, it automatically starts a second confirmation ballot (to prevent last minute approvals)
        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 );
}

which in turn tries to create a confirmation proposal via this function:

function createConfirmationProposal( 
        string calldata ballotName, 
        BallotType ballotType, 
        address address1, 
        string calldata string1, 
        string calldata description 
        ) external returns (uint256 ballotID)
        {
        require( msg.sender == address(exchangeConfig.dao()), "Only the DAO can create a confirmation proposal" );

        return _possiblyCreateProposal( ballotName, ballotType, address1, 0, string1, description );
        }

which calls _possiblyCreateProposal in Proposals.sol

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

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

However, nothing stops a regular user from creating a proposal that ends with "_confirm" as soon as he sees a proposal for SET_CONTRACT or SET_WEBSITE_URL.

For example, if the proposal for setting a contract or a website is called "ProposalName"

He'd create a brand new proposal that ends with "_confirm" i.e "ProposalName_confirm" and these checks will pass:

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

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

The 1st one will pass because there won't be a proposal with the same name. The 2nd one will also pass because there is no proposal named "ProposalName_confirm_confirm"(after concatenating another _confirm at the end).

However, after that, the real confirmation proposal will fail on this check:

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

Because a proposal with the name "ProposalName_confirm" already exists so the value won't be 0.

Tools Used

Manual review

Recommended Mitigation Steps

You could prevent users from creating proposals that end with "_confirm". You can use a helper function like that that returns true if the proposal doesn't end with "_confirm":

function isNotConfirmSuffix(string memory str) internal pure returns (bool) {
    bytes memory strBytes = bytes(str);
    if (strBytes.length < 8) {
        return true; // String is too short to have "_confirm" as a suffix
    }

    bytes memory confirmSuffix = bytes("_confirm");
    for(uint i = 0; i < 8; i++) {
        if (strBytes[strBytes.length - 8 + i] != confirmSuffix[i]) {
            return true; // Suffix does not match "_confirm"
        }
    }

    return false; // Suffix matches "_confirm"
}

Then, when creating a proposal:

require(isNotConfirmSuffix(proposalName), "Name ends with _confirm");

Assessed type

DoS

[c4-judge]

Picodes marked the issue as duplicate of #620

[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 satisfactory