Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The unchecked return values of the call() method in the UTBExecutor contract pose a potential risk of fund lock or incomplete operations in swapAndExecute or bridgeAndExecute. #526

Closed
c4-bot-3 opened this issue Jan 23, 2024 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-25 insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-3
Copy link
Contributor

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
https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/UTBExecutor.sol#L70

Vulnerability details

swapAndExecute and bridgeAndExecute are two pivotal functions in the Decent protocol, outlined in the UTB contract. Both functions invoke the execute() function within the UTBExecutor contract through the _swapAndExecute function of the UTB contract. This call is integral to the execution of a payment transaction involving native and/or ERC20 tokens.

And the Issue is, the execute() function within the UTBExecutor contract contains several external calls to a target contract and ETH transfer to refund address without proper checks for the return values. This can lead to two critical issues: funds may become stuck due to a failed refund, and the failure of the payment execution call may go unnoticed, potentially resulting in incomplete operations or loss of funds.

Impact

The impact of this finding is twofold:

  1. Funds Stuck Due to Failure of Refund: If the call to the target contract fails, the contract attempts to refund the native currency to the refund address. Without checking the success of this refund, funds could be permanently locked in the contract, leading to a direct financial loss for users.

  2. Incomplete Operation or Loss of Funds: The lack of checks after calling target.call(payload) means that if the call fails, the contract does not revert the transaction. This could result in the ERC20 tokens being locked or lost, and the intended operation not being executed, which is misleading and potentially damaging to users.

This vulnerability permits the UTB contract to perform a currency swap from the incoming to the outgoing token and initiate a transaction involving payment. However, it occurs without actually executing the proper payment or refund and without carrying out the execution of functions within the target contract.

Proof of Concept

The execute() function within the UTBExecutor does not check the return value of the refund.call{value: amount}("") or refund.call{value: extraNative}(""), which can lead to funds being stuck in the contract.

The contract does not check the return value of target.call(payload), which can lead to an incomplete operation or loss of funds.

The following code snippets from the UTBExecutor contract demonstrate the issues:

  1. Unchecked refund after failed payment with native currency in line number 54:
52:    (success, ) = target.call{value: amount}(payload);
53:    if (!success) {
54: @>     refund.call{value: amount}(""); // @audit
55:    }
56:    return;

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

  1. Unchecked refund after failed payment with additional native currency in line number 67:
65:    (success, ) = target.call{value: extraNative}(payload);
66:    if (!success) {
67: @>     refund.call{value: extraNative}(""); // @audit
68:    }

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

  1. Unchecked payment execution call in line number 70:
70: @> (success, ) = target.call(payload); // @audit

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

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate these issues, the following steps are recommended:

  1. Implement checks for the return value of the refund calls and revert the transaction if the refund fails. This ensures that either the operation succeeds, or the funds are safely returned to the user.

  2. Implement checks for the return value of the payment execution call and revert the transaction if the call fails. This ensures that the operation either completes successfully or is entirely rolled back, preventing any loss of funds.

Example fixes for the issues are as follows:

For unchecked refund:

    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}(""));
+               require(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}(""));
+               require(refund.call{value: extraNative}(""));
            }
        } else {
            (success, ) = target.call(payload);
+           require(success);
        }

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

        if (remainingBalance == 0) {
            return;
        }

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

Implementing these checks will significantly improve the contract's reliability and safeguard users' funds against potential losses due to failed transactions.

Assessed type

Invalid Validation

@c4-bot-3 c4-bot-3 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jan 23, 2024
c4-bot-8 added a commit that referenced this issue Jan 23, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Jan 25, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #25

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Feb 2, 2024
@c4-judge
Copy link

c4-judge commented Feb 2, 2024

alex-ppg marked the issue as unsatisfactory:
Invalid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-25 insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants