Return value of low level call not checked

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTBExecutor.sol#L67-L67
https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTBExecutor.sol#L70-L70

Vulnerability details

Impact

The function in question contains two lines where low-level calls are made and their return values are not checked:

1. Refund with extraNative without checking the return value:

  (refund.call{value: extraNative}(""));

In this line, extraNative currency is attempted to be sent to the refund address using a low-level call. The issue with this line is that it does not check the success of the call. If the call fails, the contract will not be aware of it, and no revert will occur. This can lead to a situation where the extraNative funds are not successfully refunded, but the contract continues execution as if they were.

2. Target call without checking the return value:

  (success, ) = target.call(payload);

This line is performing a call to the target address without sending any native currency and assigning the success status to a variable success. However, after this assignment, there is no accompanying check to determine whether success is true or false. If this call fails (i.e., success is false), the contract will not be aware of it, and subsequent code may execute under the false premise that the call was successful. This could lead to unintended behavior and state inconsistency within the contract.

For both issues, the lack of checking the return value poses risks such as loss of funds (in the case of a failed refund) and incorrect execution flow (if the contract continues after a failed call to target).

Proof of Concept

Code

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";

contract Caller {

    Vault private vaultAddress;
    constructor(Vault _vaultAddr) payable {
        vaultAddress = _vaultAddr;
    }

    receive() payable external {
        revert();
    }

    function claim(uint256 amount) public {
        vaultAddress.claimEther(amount);
    }

}

contract Vault {
    Caller private callerContract;
    constructor() payable {}

    receive() payable external {
        revert();
    }

    function claimEther(uint256 amount) public {
        require(amount <= 3 ether, "Cannot claim more than 3 ether");
        msg.sender.call{value:amount}("");
        //(bool sent,) = msg.sender.call{value: amount}("");
        //require(sent, "sent failed");
    }

    function sendEither() payable external {
        callerContract.claim(3 ether);
    }

    function setCaller(Caller _caller) public {
        callerContract = _caller;
    }

}

contract UncheckedCallTest is Test {

    address public Bob;
    Vault public vault;
    Caller public caller;

    function setUp() public {
        Bob = makeAddr("Bob");
        vault = new Vault();
        caller = new Caller(vault);
        vault.setCaller(caller);
        deal(Bob, 10 ether);
    }

    function test_callReturnNotChecked() public {
        console2.log("Before calling: Bob balance is %d ether", Bob.balance / 1e18);
        console2.log("Before calling: Vault balance is %d ether", address(vault).balance / 1e18);
        console2.log("Bob sends 3 ether");
        vm.startPrank(Bob);
        vault.sendEither{value: 3 ether}();
        console2.log("After  calling: Bob balance is %d ether", Bob.balance / 1e18);
        console2.log("After  calling: Vault balance is %d ether", address(vault).balance / 1e18);
        vm.stopPrank();

    }
}

Output

Running 1 test for test/audit/UncheckCall.t.sol:UncheckedCallTest
[PASS] test_callReturnNotChecked() (gas: 42240)
Logs:
  Before calling: Bob balance is 10 ether
  Before calling: Vault balance is 0 ether
  Bob sends 3 ether
  After  calling: Bob balance is 7 ether
  After  calling: Vault balance is 3 ether

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.33ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Ether transfer to the initiating contract does not succeed, yet the transaction does not revert, consequently causing the ether to remain in the Vault contract.

Incorporating a mechanism to verify the outcome of the call could avert this problem.

    function claimEther(uint256 amount) public {
        require(amount <= 3 ether, "Cannot claim more than 3 ether");
        msg.sender.call{value:amount}("");
        (bool sent,) = msg.sender.call{value: amount}("");
        require(sent, "sent failed");
    }

Running 1 test for test/audit/UncheckCall.t.sol:UncheckedCallTest
[FAIL. Reason: revert: sent failed] test_callReturnNotChecked() (gas: 46815)
Logs:
  Before calling: Bob balance is 10 ether
  Before calling: Vault balance is 0 ether
  Bob sends 3 ether

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.37ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/audit/UncheckCall.t.sol:UncheckedCallTest
[FAIL. Reason: revert: sent failed] test_callReturnNotChecked() (gas: 46815)

Tools Used

Manual review

Recommended Mitigation Steps

It is imperative that these calls are followed by checks on their return values, and appropriate actions are taken if they are false. Without these checks, the contract is at risk of executing further logic based on the assumption that previous operations were successful when they may not have been, potentially leading to inconsistencies in contract state and unintended loss of funds.

For the first case:

(bool refundSuccess, ) = refund.call{value: extraNative}("");
require(refundSuccess, "Refund of extraNative failed");

For the second case:

(success, ) = target.call(payload);
require(success, "Call to target failed");

Assessed type

call/delegatecall

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

This has been reported by several other bot reports. It should be deemed inconsequential because refund is typically the EOA that initiated the transaction according to the function NatSpec.

[alex-ppg]

This and all relevant exhibits detail that low-level call results are improperly handled.

In all referenced instances, either the success variable is correctly handled or a refund is performed in which case (as detailed in #206) I consider it deliberate to not induce failure due to the non-blocking nature of cross-chain messages.

Based on the above, I consider this and all relevant exhibits as invalid. #641 is also kind of similar and can be consulted for a separate judgment of the same type of issue.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[mcgrathcoutinho]

Hi @alex-ppg, thank you for the detailed response.

I think this is still an issue since even if calls are intentionally not checked to respect the non-blocking pattern, they would still cause locking of those funds in the UTBExecutor contract. There is no withdrawal mechanism implemented to rescue those funds.

Since this falls under the fund accumulation attack idea suggested in the README as well, please consider re-evaluating this issue.

Thank you for your time.

[alex-ppg]

Hey @mcgrathcoutinho, the contract is unable to confirm the transfer of funds unless it results in a blocking pattern.

Given that a blocking pattern would introduce a high-severity vulnerability while funds lost due to a misconfigured refund recipient by the user is arguable, it is advisable for the project to retain their current behavior rendering my original ruling to remain valid.