Persistent Contract Call revert prevents finalizing a ballot

mediumCode4rena

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:

solidity📋
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.

solidity📋
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:

solidity📋
// 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:

solidity📋
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:

solidity📋
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:

solidity📋
    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