Use of unchecked low level calls

[c4-bot-9]

Lines of code

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

Vulnerability details

In the UTBExecutor contract, the execute function performs external calls using the low-level call method, which can be unsafe, especially if return values are not checked:

function execute(
    // ...
) public onlyOwner {
    if (token == address(0)) {
        (success, ) = target.call{value: amount}(payload);
        // ...
    }
    // ...
    if (extraNative > 0) {
        (success, ) = target.call{value: extraNative}(payload);
        // ...
    }
}

Impact

• The lack of return value checks for the call method means that the contract cannot ascertain whether the external call was successful. This could lead to unhandled failures in execution.

• The call method can also expose the contract to reentrancy attacks, as it allows the called contract to execute arbitrary code.

• If the external call fails but the contract does not handle this failure, funds sent with the call may be lost or locked.

Mitigation

To address these risks, consider the following modifications:

• Always check the return value of the call method to ensure the external call was successful.

     function execute(
         // ... existing parameters ...
     ) public onlyOwner {
         bool success;
         if (token == address(0)) {
             (success, ) = target.call{value: amount}(payload);
             require(success, "External call failed");
         }
         // ...
         if (extraNative > 0) {
             (success, ) = target.call{value: extraNative}(payload);
             require(success, "External call with extraNative failed");
         }
         // ...
     }

• Implement a reentrancy guard to prevent reentrancy attacks.

     bool private locked;

     modifier nonReentrant() {
         require(!locked, "ReentrancyGuard: reentrant call");
         locked = true;
         _;
         locked = false;
     }

     function execute( ... ) public payable onlyOwner nonReentrant {
         // ...
     }

• Instead of using the low-level call, consider using higher-level abstractions provided by Solidity (like transfer or send for Ether transfers, or directly calling known functions on the target contract).

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

[raymondfam]

Non-specific addressing of the unchecked return impact.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid