Persistent Contract Call revert prevents finalizing a ballot

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L180 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L219 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L219

Vulnerability details

Description

The DAO._executeApproval function does not handle an external contract call error:

else if (ballot.ballotType == BallotType.CALL_CONTRACT) {
    // @audit-issue unhandled revert
    ICalledContract(ballot.address1).callFromDAO(ballot.number1);

    emit ContractCalled(ballot.address1, ballot.number1);
}

Given an approved CALL_CONTRACT ballot that can be finalized, the ballot won't be marked as finalized if the external contract call (from above) reverts.

function _finalizeApprovalBallot(uint256 ballotID) internal {
    if (proposals.ballotIsApproved(ballotID)) {
        Ballot memory ballot = proposals.ballotForID(ballotID);
        _executeApproval(ballot);
    }

    // @audit-issue the line below won't be executed if `_executeApproval` reverts
    proposals.markBallotAsFinalized(ballotID);
}

Impact

A permanent revert leaves the ballot unfinalized, and the user that posted it with an active proposal (in the _userHasActiveProposal mapping); a state that prevents the user account from creating a new proposal. At this point the user has two options to sort out the situation:

A. Unstake and transfer its SALT into a new account. B. Convince the other users to reach quorum on NO and finalize the ballot without calling the external contract.

Proof Of Concept

New contract to be created in /src/dao/tests:

// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "../interfaces/ICalledContract.sol";

contract TestCallReceiverFaulty is ICalledContract {
    uint256 public value;

    function callFromDAO(uint256 n) external {
        value = n;
        revert("callFromDAO() unexpectedly reverted");
    }
}

Add the following test in DAO.t.sol:

function testCallContractApproveReverted() public {
    // Arrange
    vm.startPrank(alice);
    staking.stakeSALT(1000000 ether);

    TestCallReceiverFaulty testReceiver = new TestCallReceiverFaulty();

    uint256 ballotID = proposals.proposeCallContract(
        address(testReceiver),
        123,
        "description"
    );
    assertEq(
        proposals.ballotForID(ballotID).ballotIsLive,
        true,
        "Ballot not correctly created"
    );

    proposals.castVote(ballotID, Vote.YES);
    vm.warp(block.timestamp + 11 days);

    vm.expectRevert("callFromDAO() unexpectedly reverted");

    // Act
    dao.finalizeBallot(ballotID);

    // Assert
    assertEq(
        proposals.ballotForID(ballotID).ballotIsLive,
        true,
        "Ballot not correctly finalized"
    );
    assertTrue(
        testReceiver.value() != 123,
        "Receiver shouldn't receive the call"
    );
    assertEq(
        proposals.userHasActiveProposal(alice),
        true,
        "Alice proposal is not active"
    );
}

Tools Used

Test and manually reviewed.

Recommended Mitigation Steps

A trivial solution is to handle the reverted external contract call with a try..catch and allow to always mark the approved ballot as finalized. A new ballot can always be created if the desired effects of the call were not applied on the first call.

Amend the CALL_CONTRACT case in the DAO._executeApproval function:

else if (ballot.ballotType == BallotType.CALL_CONTRACT) {
    try ICalledContract(ballot.address1).callFromDAO(ballot.number1) {
        // NB: place the emission outside if it must be emitted no matter the external call outcome
        emit ContractCalled(ballot.address1, ballot.number1);
    } catch (bytes memory) {}
}

Add the following test in DAO.t.sol:

    function testCallContractApproveRevertHandled() public {
        // Arrange
        vm.startPrank(alice);
        staking.stakeSALT(1000000 ether);

        TestCallReceiverFaulty testReceiver = new TestCallReceiverFaulty();

        uint256 ballotID = proposals.proposeCallContract(
            address(testReceiver),
            123,
            "description"
        );

        // Act
        _voteForAndFinalizeBallot(ballotID, Vote.YES);

        // Assert
        assertTrue(
            testReceiver.value() != 123,
            "Receiver shouldn't receive the call"
        );
        assertEq(
            proposals.userHasActiveProposal(alice),
            false,
            "Alice proposal is not active"
        );
    }

Assessed type

Governance

[Picodes]

Related to #622

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

callFromDAO now wrapped in a try/catch

https://github.com/othernet-global/salty-io/commit/5f1a5206a04b0f3fe45ad88a311370ce12fb0135

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[fnanni-0]

I think this is a duplicate of #362.

From Code4rena docs:

Similar exploits under a single issue

Findings are duplicates if they share the same root cause.

More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

The vulnerabilty's root cause of both #362 and #1009 is that proposals don't expire. It's not a problem in itself that proposals might not reach quorum or that they might revert. Both things are expected behaviors of most governance contracts and simply adding an expiration/cancelling mechanism fixes them.

Note that at least one submission (#490) mentioned both ramifications of the issue but was split in two.

On the other hand, I'm not convinced that adding a try/catch and finalizing failed proposals is a good idea. If a proposal's execution could be DoS by making the target call unavailable even for just a block, the proposal would end, though it would be a lot harder to DoS it during a long time. Look at other governance systems and probably most won't bother with transactions reverting. A few examples: OpenZeppelin Governor, Zodiac's RealityModule, Kleros Governor. IMO proposals not expiring should be fixed, not transactions failing.

[Picodes]

@fnanni-0 thanks for your comment. I still think there are 2 distinct issues here although as you're correctly pointing out we could wrap them in a larger one.

• 362 is to me mainly about the fact that proposals that don't reach quorum can't be finalized and the fact that in some cases you can't call finalize because of https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L385

• This issue is about the fact that sometimes the underlying call reverts so you can't call finalize either

  We can imagine adding a function to fix both issues at once, but most suggested mitigations only fix one.

Most reports discuss only one of these 2 things, so to me it's fair to consider there are 2 separate issues.