Mystery Box

Vulnerability Details

The MysteryBox::claimSingleReward contains an off-by-one error in array indexing, combined with insufficient reentrancy protection. This combination allows an attacker to potentially drain funds from the contract by exploiting the faulty array bounds check and repeatedly calling the vulnerable function before the state is updated.

POC

1. Off-by-one error: userIndex[msg.sender] is initialized to balances.length, which is one index beyond the array's valid range.

2. Lack of reentrancy protection: The balance is updated after the transfer, allowing for potential reentrancy attacks.

An attacker can exploit this by:

1. Calling deposit() to initialize their balance.

2. Calling withdraw() with a small amount, triggering the vulnerable state.

3. In the fallback function, repeatedly calling withdraw() before the balance is updated.

Impact

1. Unauthorized withdrawal of funds

2. Potential complete drainage of contract balance

3. Loss of user trust and financial damage to the protocol

Tools Used

Manual Review, Foundry

Recommendations

Fix the off-by-one error: The condition require(_index <= rewardsOwned[msg.sender].length, "Invalid index"); allows _index to be equal to the length of the array. However, array indices are zero-based, so the valid indices should be from 0 to rewardsOwned[msg.sender].length - 1.
To fix this, change the condition to

Reentrancy protection: It’s best practice to update state (e.g., delete rewardsOwned[msg.sender][_index];) before transferring Ether to avoid potential reentrancy attacks.
You can reorder the code like this:

By clearing the reward first, you protect the contract from malicious external calls that could exploit the Ether transfer.

I recommend checking all instances in the contract where Ether is transferred and ensuring this pattern is followed.