Not checking the return state of the calls in UTBExecutor::execute can cause loss/locking of funds

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/UTBExecutor.sol#L54
https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/UTBExecutor.sol#L67

Vulnerability details

Impact

The function UTBExecutor::execute does not handle the return value of the calls refund.call{value: amount}("") and refund.call{value: extraNative}(""), which could lead to a loss of funds for users. This is because if the refund call fails, the contract does not revert the transaction, and the funds intended for refund remain in the contract.

This behavior violates one of the protocol's invariants which is the prevention of the accumulation of funds in contracts other than UTBFeeCollector and DcntEth :

Fund Accumulation: Other than the UTBFeeCollector, and DcntEth, the contracts are not intended to hold on to any funds or unnecessary approvals. Any native value or erc20 flowing through the protocol should either get delivered or refunded.

Proof of Concept

The issue lies in the following line of code:

    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}(""));//@audit not check success state
            }
        } else {
            (success, ) = target.call(payload);
        }

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

        if (remainingBalance == 0) {
            return;
        }

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

In both calls, if the call to the refund address fails, the contract does not revert or handle the failure. This can result in the funds being unintentionally locked in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Check and handle the success state of the call :

diff --git a/src/UTBExecutor.sol b/src/UTBExecutor.sol
index 38c3f8a..d1049d5 100644
--- a/src/UTBExecutor.sol
+++ b/src/UTBExecutor.sol
@@ -51,7 +51,8 @@ contract UTBExecutor is Owned {
         if (token == address(0)) {
             (success, ) = target.call{value: amount}(payload);
             if (!success) {
-                (refund.call{value: amount}(""));
+                (success, ) = (refund.call{value: amount}(""));
+                require(success, "Refund failed");
             }
             return;
         }
@@ -64,7 +65,8 @@ contract UTBExecutor is Owned {
         if (extraNative > 0) {
             (success, ) = target.call{value: extraNative}(payload);
             if (!success) {
-                (refund.call{value: extraNative}(""));
+                (success, ) =  (refund.call{value: extraNative}(""));
+                require(success, "Refund failed");
             }
         } else {
             (success, ) = target.call(payload);

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 #25

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid