Unhandled Ether Transfer

[codehawks-bot]

Unhandled Ether Transfer

Severity

Medium Risk

Summary

Tests assert that the Ether balances of the proxies remain at zero, but the contracts may receive Ether due to the lack of proper handling.

Vulnerability Details

The "ProxyTest" contract contains a test function, "testIfSendEtherToProxyThenRevert," where Ether is sent to instances of the "Proxy" contract using the .call{value: ...}("") syntax. However, the test cases do not properly handle the revert condition that should occur when trying to send Ether to a contract that does not have a payable fallback function. As a result, the tests assert that the Ether balances of the proxies remain at zero, but the contracts may receive Ether due to the lack of proper handling.

function testIfSendEtherToProxyThenRevert() public {
    vm.deal(msg.sender, 2 ether);
    vm.expectRevert();
    (bool success,) = address(proxy).call{value: 1 ether}("");
    // no ether arrived
    assertEq(0, address(proxy).balance);

    (bool success2,) = address(secondProxy).call{value: 1 ether}("");
    // no ether arrived
    assertEq(0, address(secondProxy).balance);
}

Impact

The lack of proper handling of Ether transfers to contracts that do not have a payable fallback function can lead to unintended behavior and security vulnerabilities. This could result in loss of Ether sent to the contracts, as well as potential disruption of the contracts' functionality.

Tools Used

Manual

Recommendations

1. Update the test cases to include assertions that check for the proper handling of Ether transfers to the proxy contracts that lack a payable fallback function.
2. Use try-catch or require statements to handle the revert condition and verify that Ether transfers to non-payable contracts are properly rejected. By addressing this vulnerability and ensuring proper handling of Ether transfers, you can prevent unintended Ether transfers and potential disruptions to contract behavior.

function testIfSendEtherToProxyThenRevert() public {
    // Attempt to send Ether to the proxy contract without a payable fallback
    try address(proxy).call{value: 1 ether}("") {
        // Revert the test and fail if Ether transfer was successful
        revert("Ether transfer should have reverted");
    } catch Error(string memory /*reason*/) {
        // Ether transfer should have reverted, continue testing
    } catch {
        // Unexpected error occurred, fail the test
        revert("Unexpected error occurred");
    }

    // Perform the same checks for the secondProxy contract
    try address(secondProxy).call{value: 1 ether}("") {
        // Revert the test and fail if Ether transfer was successful
        revert("Ether transfer should have reverted");
    } catch Error(string memory /*reason*/) {
        // Ether transfer should have reverted, continue testing
    } catch {
        // Unexpected error occurred, fail the test
        revert("Unexpected error occurred");
    }

    // Additional test logic...
}

In these code snippets, the try statement is used to execute the Ether transfer and catch any exceptions that may occur. If the transfer was successful, the test case is reverted with an appropriate error message. If the transfer resulted in a revert due to the lack of a payable fallback function, the test case continues. This ensures that the test cases explicitly verify the behavior of Ether transfers to contracts without a payable fallback.

By adding these try-catch blocks, you can properly handle Ether transfers and prevent unintended transfers to contracts that are not designed to accept Ether.

[PatrickAlphaC]

out of scope