DatingDapp
First Flight #33
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Incorrect Return Value Handling in ETH Transfers

sauronsol

Summary

The contract does not properly handle return values from ETH transfers, making it vulnerable to false positive returns that could lead to inconsistent state.

Vulnerability Details

The contract assumes that a successful return value from a low-level call means the ETH transfer succeeded, but this can be manipulated:

  1. A malicious contract can revert with a message that looks like true

  2. The contract doesn't verify the actual balance changes

  3. This can lead to state inconsistencies

function matchRewards(address from, address to) internal {
// ... state changes ...
​
// Only checks the return value, which can be manipulated
(bool success,) = payable(address(multiSigWallet)).call{value: rewards}("");
require(success, "Transfer failed"); // Can be fooled by malicious return
}

Impact

  1. State Inconsistency : Contract state could be updated even when ETH transfer actually failed

  2. False Success Reports : External integrations could be misled about transfer success

  3. Potential for Manipulation : Malicious contracts could exploit this behavior

Severity: MEDIUM - Can lead to state inconsistencies but requires specific conditions

Tools Used

  • Foundry for testing

  • Manual code review

  • PoC demonstrating the false positive return

Proof of Concept

contract FalsePositiveMultiSig {
address public owner1;
address public owner2;
​
constructor(address _owner1, address _owner2) {
owner1 = _owner1;
owner2 = _owner2;
}
​
receive() external payable {
// Revert with a message that looks like true
revert("0x0000000000000000000000000000000000000000000000000000000000000001");
}
}
​
function testFalsePositiveTransfer() public {
console.log("Starting false positive transfer test");
​
// Setup initial state
vm.deal(alice, 2 ether);
vm.deal(bob, 2 ether);
​
// Alice likes Bob
vm.prank(alice);
likeRegistry.likeUser{value: 1 ether}(bob);
​
// Deploy malicious MultiSig that reverts with true-like message
FalsePositiveMultiSig falsePositiveMultiSig = new FalsePositiveMultiSig(alice, bob);
​
// Verify that the contract doesn't accept ETH but returns true
uint256 initialBalance = address(falsePositiveMultiSig).balance;
vm.expectRevert("0x0000000000000000000000000000000000000000000000000000000000000001");
(bool success,) = address(falsePositiveMultiSig).call{value: 1 ether}("");
// Verify ETH was not accepted
assertEq(
address(falsePositiveMultiSig).balance,
initialBalance,
"Contract should not have received ETH"
);
}

Recommendations

  1. Verify actual balance changes after transfers:

function matchRewards(address from, address to) internal {
// ... other code ...
​
uint256 initialBalance = address(multiSigWallet).balance;
(bool success,) = payable(address(multiSigWallet)).call{value: rewards}("");
require(success, "Transfer failed");
// Verify actual balance change
require(
address(multiSigWallet).balance == initialBalance + rewards,
"ETH transfer verification failed"
);
}

  1. Use OpenZeppelin's Address.sendValue():

using Address for address payable;
​
function matchRewards(address from, address to) internal {
// ... other code ...
payable(address(multiSigWallet)).sendValue(rewards);
}

  1. Implement a try-catch pattern for transfers:

function matchRewards(address from, address to) internal {
// ... other code ...
uint256 initialBalance = address(multiSigWallet).balance;
try payable(address(multiSigWallet)).call{value: rewards}("") returns (bool success) {
require(success, "Transfer failed");
require(
address(multiSigWallet).balance == initialBalance + rewards,
"ETH not received"
);
} catch (bytes memory reason) {
revert(string(abi.encodePacked("Transfer failed: ", reason)));
}
}
Updates

Appeal created

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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