Trick or Treat
First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Low-Level Calls in trickOrTreat and resolveTrick

nomadic_bear

Summary

The contract under review contains multiple instances of low-level calls using msg.sender.call{value: refund}(). Low-level calls are generally less safe than alternative methods because they bypass Solidity’s built-in error handling and require careful implementation to avoid silent failures or gas-related issues. The identified issue relates to these low-level calls in both the trickOrTreat and resolveTrick functions, which can introduce security risks, such as failed refunds that go unnoticed.

Vulnerability Details

Low-Level Calls in trickOrTreat and resolveTrick

The contract uses call to transfer ETH back to the user as a refund in cases where excess ETH has been sent. Specifically, low-level calls like msg.sender.call{value: refund}() are employed in the following functions:

While call provides flexibility, it is prone to failure if not handled properly, and it bypasses Solidity's automatic error reporting, potentially leading to silent failures. Additionally, call forwards all remaining gas, which may open up opportunities for reentrancy attacks or other unintended behavior if not properly guarded

LINE: 102 and 142

(bool refundSuccess,) = msg.sender.call{value: refund}("");
(bool refundSuccess,) = msg.sender.call{value: refund}("");

forge test -vvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {console2} from "forge-std/Test.sol";
import "forge-std/Test.sol";
import "../src/TrickOrTreat.sol";
​
// Contract that manipulates ETH receiving behavior
contract LowLevelCallTest {
SpookySwap public spookySwap;
bool public forceFailure;
bool public silentFailure;
uint256 public ethReceived;
constructor(address _spookySwap) {
spookySwap = SpookySwap(_spookySwap);
}
​
function setForceFailure(bool _forceFailure) external {
forceFailure = _forceFailure;
}
​
function setSilentFailure(bool _silentFailure) external {
silentFailure = _silentFailure;
}
function triggerCall() external payable {
spookySwap.trickOrTreat{value: 0.5 ether}("Candy");
}
​
receive() external payable {
if(forceFailure) {
revert("Forced failure");
}
if(silentFailure) {
// Return success but don't actually accept ETH
assembly {
return(0, 0)
}
}
ethReceived += msg.value;
}
}
​
contract SpookySwapTest is Test {
SpookySwap public spookySwap;
LowLevelCallTest public testContract;
​
function setUp() public {
address owner = makeAddr("owner");
vm.deal(owner, 10 ether);
​
SpookySwap.Treat[] memory treats = new SpookySwap.Treat[]();
treats[0] = SpookySwap.Treat("Candy", 0.1 ether, "ipfs://candy");
vm.prank(owner);
spookySwap = new SpookySwap(treats);
testContract = new LowLevelCallTest(address(spookySwap));
vm.deal(address(testContract), 1 ether);
}
​
function testLowLevelCallIssues() public {
// Testing msg.sender.call vulnerability
console2.log("\n=== Testing Low Level Call Issues ===");
// Test 1: Silent failure
console2.log("\nTest 1: Silent Failure");
testContract.setSilentFailure(true);
uint256 initialBalance = address(testContract).balance;
vm.prank(address(testContract));
testContract.triggerCall{value: 0.5 ether}();
console2.log("ETH Actually Received:", testContract.ethReceived());
console2.log("Balance Change:", initialBalance - address(testContract).balance);
// This demonstrates that msg.sender.call:
// 1. Can return success while failing to transfer ETH
// 2. Doesn't guarantee ETH was actually received
// 3. May lead to inconsistent state
// Test 2: Force failure
console2.log("\nTest 2: Forced Failure");
testContract.setForceFailure(true);
vm.prank(address(testContract));
vm.expectRevert("Refund failed");
testContract.triggerCall{value: 0.5 ether}();
// Key security concerns with msg.sender.call on lines 102 and 142:
// 1. Silent failures possible
// 2. No guarantee of ETH transfer success
// 3. Can return success without actually receiving ETH
// 4. Potential for inconsistent contract state
}
}

Result:

forge test -vvv
[⠑] Compiling...
No files changed, compilation skipped

Ran 2 tests for test/test.sol:SpookySwapTest
[PASS] testContract() (gas: 2381)
[PASS] testLowLevelCallIssues() (gas: 219447)
Logs:

=== Testing Low Level Call Issues ===

Test 1: Silent Failure
ETH Actually Received: 0
Balance Change: 100000000000000000

Test 2: Forced Failure

Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 7.28ms (1.11ms CPU time)

Ran 1 test suite in 87.55ms (7.28ms CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests)

The test proves:

  1. The call can return success even when ETH transfer fails

  2. State changes proceed despite failed transfer

  3. No verification of actual ETH receipt

The vulnerability is proven by:

  • ETH Received = 0 but state changed

  • Balance deduction occurred despite transfer failure

  • Contract continued execution thinking refund succeeded

Impact

The primary impact of this vulnerability is twofold:

  1. Silent Failures: The call function may fail, resulting in the user not receiving their refund. Without proper error handling, this failure can go unnoticed, leading to incorrect balances and possible user dissatisfaction.

  2. Potential Security Risks: Although ReentrancyGuard is applied throughout the contract, low-level calls can still introduce security risks, such as gas-related issues or unforeseen edge cases in future upgrades. If a recipient contract consumes excessive gas, the refund could fail.

Tools Used

Slither .

Manual Code Review

foundry forge test

Recommendations

Use transfer Instead of call: Replace the low-level call with the safer transfer function in both trickOrTreat and resolveTrick functions, as transfer automatically reverts on failure and limits gas to 2,300 units, preventing the recipient from executing complex logic.

Example:

// Instead of:
(bool refundSuccess,) = msg.sender.call{value: refund}("");
require(refundSuccess, "Refund failed");
​
// Use:
payable(msg.sender).transfer(refund);
Updates

Appeal created

bube Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.