Mystery Box

Summary

The MysteryBox.sol contract contains a critical off-by-one error in the MysteryBox::claimSingleReward function. This vulnerability allows users to access an out-of-bounds index in their rewards array, potentially leading to unexpected behavior, including claiming non-existent rewards or causing the function to revert unexpectedly.

Vulnerability Details

Affected code - https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L92-L101

The vulnerability stems from an incorrect boundary check in the MysteryBox::claimSingleReward function:

The logic flaw is in the first require statement:

• It uses <= instead of < when comparing the index to the array length.

• This allows accessing an index equal to the array length, which is out of bounds in Solidity (as array indices start at 0).

PoC

Here's a textual proof of concept demonstrating the vulnerability:

1. A user accumulates one or more rewards through the openBox function.

2. The user calls ``MysteryBox::claimSingleReward` with an index equal to the length of their rewards array.

3. The function passes the first require check (_index <= rewardsOwned[msg.sender].length).

4. When attempting to access rewardsOwned[msg.sender][_index], the contract tries to read from an out-of-bounds array index.

5. This results in either:
   a) The function reverting due to an out-of-bounds access, or
   b) Returning a default (empty) reward value, which then fails the require(value > 0, "No reward to claim") check.

In either case, the function behaves unexpectedly, allowing an invalid index to pass the initial check.

Impact

The vulnerability in the MysteryBox::claimSingleReward function can lead to several issues:

1. Unexpected Reverts: Users may experience transaction failures when attempting to claim the "last+1" reward, leading to confusion and poor user experience.

2. Potential for Exploitation: In certain scenarios, this could be exploited to manipulate the reward claiming process or cause unexpected state changes in the contract.

3. Inconsistent Behavior: The function may behave inconsistently depending on the specific implementation and compiler version, making the contract's behavior unpredictable.

4. Gas Waste: Failed transactions due to this error will waste gas, potentially causing financial losses for users.

While not immediately resulting in fund loss, this vulnerability represents a significant logic flaw that undermines the reliability and correctness of the reward claiming process.

Tools Used

Manual Review

Recommendations

To address this vulnerability, the boundary check in the MysteryBox::claimSingleReward function should be corrected:

This change ensures that only valid array indices can be accessed, preventing out-of-bounds access and maintaining the integrity of the reward claiming process.

Additionally, consider implementing the following improvements:

1. Add input validation to ensure _index is not excessively large:

   require(_index < type(uint256).max, "Invalid index");

2. Implement events to log reward claims for better transparency:

   event RewardClaimed(address indexed user, uint256 index, uint256 value);

By implementing these recommendations, the contract can prevent out-of-bounds access in the MysteryBox::claimSingleReward function, enhancing its security and reliability.