UTBExecutor.execute function doesn't revert on failure

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/main/src/UTBExecutor.sol#L69-L80

Vulnerability details

Impact:

The identified issue in the execute function poses a potential risk due to the absence of a revert mechanism in case the external call to target fails. If the external call execution fails, there is no reversion of state, which may result in funds being left in the contract without proper handling. This lack of a revert mechanism on failed external calls could lead to potential funds loss and unexpected behavior in the protocol.

Proof of Concept:

The issue is evident in the execute function, where there is no revert mechanism if the external call to target fails:

function execute(
    address target,
    address paymentOperator,
    bytes memory payload,
    address token,
    uint amount,
    address payable refund,
    uint extraNative
) public onlyOwner {
    bool success;
    if (token == address(0)) {
        (success, ) = target.call{value: amount}(payload);
        if (!success) {
            (refund.call{value: amount}(""));
        }
        return;
    }

    uint initBalance = IERC20(token).balanceOf(address(this));

    IERC20(token).transferFrom(msg.sender, address(this), amount);
    IERC20(token).approve(paymentOperator, amount);

    if (extraNative > 0) {
        (success, ) = target.call{value: extraNative}(payload);
        if (!success) {
            (refund.call{value: extraNative}(""));
        }
    } else {
        (success, ) = target.call(payload);
        //@audit doesn't revert if call fails
    }

    uint remainingBalance = IERC20(token).balanceOf(address(this)) -
        initBalance;

    if (remainingBalance == 0) {
        return;
    }

    IERC20(token).transfer(refund, remainingBalance);
}

As it can be seen when making the external call to the target contract, the bool return value success is not checked and so if that call did fail (for some reason but did not revert just returned success == false) the execute function will not revert which mean that any funds sent/taken by that target contract won't be returned to the caller leading to a financial loss.

Tools Used:

Manual review

Recommended Mitigation Steps:

Update the execute function to check the return value success and revert in case the external call to target fails success == false.

Assessed type

call/delegatecall

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #70

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[alex-ppg]

This and all duplicate submissions detail how the success variable is ignored from the external call.

The code executes as expected given that the success is meant to be ignored. While it is more gas optimal to not assign a value to it, the remainingBalance calculation that ensues will properly refund the tokens that were initially allocated to the interaction. As such, no fund loss will occur and a refund will be properly carried out under all circumstances.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid