Mystery Box
First Flight #25
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy Vulnerability in `claimAllRewards` and `claimSingleReward` Functions

0xgee001

Summary

The claimAllRewards and claimSingleReward functions are vulnerable to a reentrancy attack because they transfer Ether to the user before updating the contract’s state. This allows an attacker to repeatedly call the function and drain funds from the contract by re-entering the function before the contract's state is properly updated, leading to a depletion of the contract's balance and loss of funds.

Vulnerability Details

Both the claimAllRewards and claimSingleReward functions perform an Ether transfer to the user before updating the contract's state (deleting rewards). This exposes the contract to a reentrancy attack, where an attacker can re-enter the function during the external call (the transfer of Ether) and repeatedly claim rewards or deplete the contract's balance.

Here’s the problematic part of the code in claimAllRewards

function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
​
(bool success, ) = payable(msg.sender).call{value: totalValue}(""); // Ether is sent before state update
require(success, "Transfer failed");
​
delete rewardsOwned[msg.sender]; // State is updated only after Ether is sent
}
​

Similarly, in claimSingleReward:

function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
​
(bool success, ) = payable(msg.sender).call{value: value}(""); // Ether is sent before state update
require(success, "Transfer failed");
​
delete rewardsOwned[msg.sender][_index]; // State is updated only after Ether is sent
}
​

Both functions first send Ether using the call method, and only after the transfer is complete, they update the state of the contract by deleting the rewards. This opens the door for a reentrancy attack, where a malicious contract can repeatedly call these functions during the Ether transfer (before the rewards are deleted), draining the contract of its funds.

Impact

The impact of this vulnerability is severe:

  • Full contract drain: An attacker could repeatedly call claimAllRewards or claimSingleReward and drain the contract's balance before the rewards are properly deleted, causing significant financial loss.

  • Loss of trust: Users may lose trust in the protocol if the contract’s funds are drained due to such an exploit.

Tools Used

Manual code review

Recommendations

To prevent reentrancy attacks, follow the checks-effects-interactions pattern, which ensures that the contract's state is updated before any external calls (like transferring Ether). Here’s how the functions can be rewritten:

Fix in claimAllRewards:

function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
​
delete rewardsOwned[msg.sender]; // Update state before transferring Ether
​
(bool success, ) = payable(msg.sender).call{value: totalValue}(""); // External call after state update
require(success, "Transfer failed");
}
​

Fix in claimSingleReward:

function claimSingleReward(uint256 _index) public {
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
​
delete rewardsOwned[msg.sender][_index]; // Update state before transferring Ether
​
(bool success, ) = payable(msg.sender).call{value: value}(""); // External call after state update
require(success, "Transfer failed");
}
​

Additional Recommendations:

  • Reentrancy Guard: Consider using OpenZeppelin’s ReentrancyGuard to provide additional protection against reentrancy attacks by ensuring that a function cannot be re-entered before its execution completes.

Updates

Appeal created

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`claimAllRewards` reentrancy

`claimSingleReward` reentrancy

Support

FAQs

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

Give us feedback!