Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: medium
Invalid

[M-1] Reentrancy vulnerability in `ThePredicter::withdraw` function that could lead to loss of funds

[M-1] Reentrancy vulnerability in ThePredicter::withdraw function that could lead to loss of funds

Summary:
The withdraw function in ThePredicter.sol is vulnerable to reentrancy attacks due to violating the Checks-Effects-Interactions pattern, potentially allowing malicious users to withdraw multiple times and unfairly distribute rewards.

Vulnerability Details:
The withdraw function performs state changes before making external calls:

function withdraw() public {
if (!scoreBoard.isEligibleForReward(msg.sender)) {
revert ThePredicter__NotEligibleForWithdraw();
}
// ... (score calculation and eligibility checks)
if (reward > 0) {
scoreBoard.clearPredictionsCount(msg.sender);
(bool success,) = msg.sender.call{value: reward}("");
require(success, "Failed to withdraw");
}
}

The vulnerability arises because clearPredictionsCount is called before the Ether transfer. A malicious contract could potentially re-enter withdraw during the Ether transfer.

The state update occurs before the external call, violating the Checks-Effects-Interactions pattern.

Impact:

Medium. While not directly exposing all funds to risk, this vulnerability could allow multiple withdrawals by a single user, leading to unfair reward distribution and disruption of the protocol's intended functionality.

Tools Used:

Manual Review, Forge testing framework, AI for troubleshooting

Recommended Mitigation:

Implement the CEI pattern:

function withdraw() public {
+ address player = msg.sender;
+ // Checks
- if (!scoreBoard.isEligibleForReward(msg.sender)) {
- revert ThePredicter__NotEligibleForWithdraw();
- }
+ require(scoreBoard.isEligibleForReward(player), "Not eligible for withdrawal");
+ int8 score = scoreBoard.getPlayerScore(player);
+ require(score > 0, "Score must be positive");
+ (uint256 reward, bool isEligible) = calculateReward(score);
+ require(isEligible, "Not eligible for reward");
- // ... (score calculation and eligibility checks)
+ // Effects
+ scoreBoard.clearPredictionsCount(player);
+ playerWithdrawals[player] = true; // New state variable to track withdrawals
- if (reward > 0) {
- scoreBoard.clearPredictionsCount(msg.sender);
- (bool success,) = msg.sender.call{value: reward}("");
- require(success, "Failed to withdraw");
- }
+ // Interactions
+ (bool success,) = player.call{value: reward}("");
+ require(success, "Failed to withdraw");
+ emit Withdrawal(player, reward);
}

This re-arranges the function to the correct CEI pattern. Also, there is a few other changes to aim to mitigate the reentrancy vulnerability:

  • The addition of a new state variable playerWithdrawals to track withdrawals.

  • The removal of the if (reward > 0) check, as it's now handled by the isEligible requirement.

  • The addition of an event emission for transparency.

Additionally, it would be advised to implement a reentrancy guard using OpenZeppelin's ReentrancyGuard. To do this;

  • First, import the ReentrancyGuard contract from OpenZeppelin.

  • Then, make your contract inherit from ReentrancyGuard.

  • Finally, add the nonReentrant modifier to the withdraw function.

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
- contract ThePredicter {
+ contract ThePredicter is ReentrancyGuard {
// ... existing contract code ...
- function withdraw() public {
+ function withdraw() public nonReentrant {
address player = msg.sender;
// Checks
require(scoreBoard.isEligibleForReward(player), "Not eligible for withdrawal");
int8 score = scoreBoard.getPlayerScore(player);
require(score > 0, "Score must be positive");
(uint256 reward, bool isEligible) = calculateReward(score);
require(isEligible, "Not eligible for reward");
// Effects
scoreBoard.clearPredictionsCount(player);
playerWithdrawals[player] = true;
// Interactions
(bool success,) = player.call{value: reward}("");
require(success, "Failed to withdraw");
emit Withdrawal(player, reward);
}
}

By adding the nonReentrant modifier, the ReentrancyGuard will prevent any reentrant calls to this function, providing an additional layer of protection against reentrancy attacks. This works in conjunction with the Checks-Effects-Interactions pattern to make the function even more secure.

Updates

Lead Judging Commences

NightHawK Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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