c4-bot-7 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

A malicious user calling Proposals::proposeSetContractAddress with specific names like priceFeed1, priceFeed2, priceFeed3 or accessManager, or calling Proposals::proposeSendSALT, can create a Denial of Service (DoS) for these types of proposals, effectively preventing the setting of contracts or sending of SALT token.

For proposeSetContractAddress, this is because the contract set is determined by the specific name of the contract in the ballot, which can be the same as the hash generated. Thus, a malicious user could hijack these names after 45 days of deployment.

In the case of proposeSendSALT, there's no need to send a specific name, as the function sets the name and only one user can make this proposal at the same time. A user with the minimum requiredXSalt balance can make this proposal and deny another proposal after DAOConfig::ballotMinimumDuration. They can continually front-run the proposal and send it again if other user try.

Proof of Concept

Here, it's evident that the ballotName must specifically match for any changes to be made.

function _executeSetContract(Ballot memory ballot) internal {
        bytes32 nameHash = keccak256(bytes(ballot.ballotName));

@>        if (nameHash == keccak256(bytes("setContract:priceFeed1_confirm"))) {
            priceAggregator.setPriceFeed(1, IPriceFeed(ballot.address1));
@>        } else if (nameHash == keccak256(bytes("setContract:priceFeed2_confirm"))) {
            priceAggregator.setPriceFeed(2, IPriceFeed(ballot.address1));
@>        } else if (nameHash == keccak256(bytes("setContract:priceFeed3_confirm"))) {
            priceAggregator.setPriceFeed(3, IPriceFeed(ballot.address1));
@>        } else if (nameHash == keccak256(bytes("setContract:accessManager_confirm"))) {
            exchangeConfig.setAccessManager(IAccessManager(ballot.address1));
        }

        emit SetContract(ballot.ballotName, ballot.address1);
    }

./src/dao/DAO.sol

In the case of proposing to send Salt, the ballotName is set to sendSALT. Due to protocol restrictions, only one proposal with a unique name can be submitted.

function proposeSendSALT(address wallet, uint256 amount, string calldata description)
        external
        nonReentrant
        returns (uint256 ballotID)
    {
        require(wallet != address(0), "Cannot send SALT to address(0)");

        // Limit to 5% of current balance
        uint256 balance = exchangeConfig.salt().balanceOf(address(exchangeConfig.dao()));
        uint256 maxSendable = balance * 5 / 100;
        require(amount <= maxSendable, "Cannot send more than 5% of the DAO SALT balance");

        // 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";
        return _possiblyCreateProposal(ballotName, BallotType.SEND_SALT, wallet, amount, "", description);
    }

./src/dao/Proposals.sol

Tools Used

Manual code review

Recommended Mitigation Steps

Implement a mechanism similar to the protocol used for whitelisting tokens. If a contract needs to be set or SALT sent, the trusted result should be reflected in the election and passed through the proposal process. This approach ensures that anyone with enough tokens to make the proposal can submit a contract_name, and if the correct address wins, it will be sent to the oficial proposal.

Assessed type

DoS