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

Permanent Loss of Funds due to CEI Violation in matchRewards()

sauronsol

Summary

The matchRewards() function violates the Checks-Effects-Interactions pattern by modifying states before performing ETH transfers, leading to permanent loss of funds.

Vulnerability Details

In LikeRegistry.sol, the matchRewards function:

  1. Sets the users' balances to zero before the transfer

  2. Has no rollback mechanism in case of failure

  3. Modifies critical state before external interactions

function matchRewards(address from, address to) internal {
uint256 matchUserOne = userBalances[from];
uint256 matchUserTwo = userBalances[to];
userBalances[from] = 0; // State modified before transfer
userBalances[to] = 0; // State modified before transfer
​
uint256 totalRewards = matchUserOne + matchUserTwo;
uint256 matchingFees = (totalRewards * FIXEDFEE) / 100;
uint256 rewards = totalRewards - matchingFees;
totalFees += matchingFees; // State modified before transfer
​
// Deploy a MultiSig contract for the matched users
MultiSigWallet multiSigWallet = new MultiSigWallet(from, to);
​
// External interaction after state modifications
(bool success,) = payable(address(multiSigWallet)).call{value: rewards}("");
require(success, "Transfer failed");
}

Impact

  1. Permanent Loss of Funds : If the transfer fails, the balances are already zero and the funds remain locked in the contract

  2. No Recovery Mechanism : No way to recover the funds in case of failure

  3. Reentrancy Risk : State modifications before external calls could be exploited

Severity: HIGH - Permanent loss of funds with high probability of occurrence

Tools Used

  • Foundry for testing

  • Manual code review

  • PoC demonstrating the CEI violation

Proof of Concept

function testViolationOfCEI() public {
// 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);
​
// Bob likes Alice, triggering matchRewards
vm.startPrank(bob);
vm.expectEmit(true, true, false, true);
emit Liked(bob, alice);
vm.expectEmit(true, true, false, true);
emit Matched(bob, alice);
// Trigger the match
likeRegistry.likeUser{value: 1 ether}(alice);
vm.stopPrank();
​
// Verify states are modified in wrong order:
// 1. Match is created
vm.prank(alice);
address[] memory aliceMatches = likeRegistry.getMatches();
assertEq(aliceMatches.length, 1, "Alice should have one match");
assertEq(aliceMatches[0], bob, "Alice's match should be Bob");
​
// 2. Balances are set to 0 before transfer
assertEq(likeRegistry.userBalances(alice), 0, "Alice balance should be 0");
assertEq(likeRegistry.userBalances(bob), 0, "Bob balance should be 0");
​
// 3. ETH is still in contract (transfer failed)
assertEq(address(likeRegistry).balance, 2 ether, "ETH should still be in contract");
}

Recommendations

  1. Follow the Checks-Effects-Interactions pattern:

function matchRewards(address from, address to) internal {
uint256 matchUserOne = userBalances[from];
uint256 matchUserTwo = userBalances[to];
uint256 totalRewards = matchUserOne + matchUserTwo;
uint256 matchingFees = (totalRewards * FIXEDFEE) / 100;
uint256 rewards = totalRewards - matchingFees;
​
// Deploy MultiSig first
MultiSigWallet multiSigWallet = new MultiSigWallet(from, to);
​
// Store initial balance
uint256 initialBalance = address(multiSigWallet).balance;
​
// Attempt transfer
(bool success,) = payable(address(multiSigWallet)).call{value: rewards}("");
require(success, "Transfer failed");
​
// Verify ETH was actually received
require(
address(multiSigWallet).balance == initialBalance + rewards,
"ETH not received"
);
​
// Update state after successful transfer AND verification
userBalances[from] = 0;
userBalances[to] = 0;
totalFees += matchingFees;
}

  1. Use OpenZeppelin's Address.sendValue() which includes additional safety checks:

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

  1. Implement a pull-over-push pattern for withdrawals:

mapping(address => uint256) public pendingWithdrawals;
​
function matchRewards(address from, address to) internal {
// ... calculate rewards ...
pendingWithdrawals[address(multiSigWallet)] = rewards;
emit WithdrawalReady(address(multiSigWallet), rewards);
}
​
function withdraw() external {
uint256 amount = pendingWithdrawals[msg.sender];
require(amount > 0, "No pending withdrawal");
// Update state before transfer
pendingWithdrawals[msg.sender] = 0;
// Use OpenZeppelin's sendValue
payable(msg.sender).sendValue(amount);
}
Updates

Appeal created

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_reentrancy_with_no_impact

matchRewards: Contract is created just before and is the one called. No impact. executeTransaction: CEI is followed. Emitting an event in disorder is informational in that context. withdraw: CEI is followed.

Support

FAQs

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