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

Data Drift: Inefficient Reward Transfer Leading to Array Gaps

juggernaut63

Summary

The transferReward function in the MysteryBox contract introduces inefficiencies by deleting rewards from the array without reordering or compacting it. This approach leaves gaps in the data structure, potentially causing inefficiencies and complicating future data management.

Vulnerability Details

https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L73-L77

The function uses the delete keyword to remove an element from the rewardsOwned array, which sets the element to its default value but does not reduce the array's length or reorder the remaining elements.

function transferReward(address _to, uint256 _index) public {
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
rewardsOwned[_to].push(rewardsOwned[msg.sender][_index]);
@=> delete rewardsOwned[msg.sender][_index];
}
​

The line delete rewardsOwned[msg.sender][_index]; is where the element is set to its default value, creating a gap in the array.

Impact

Gaps in the array can lead to incorrect assumptions about the data structure, potentially causing errors in logic that processes the array.

Tools Used

Manual review

Recommendations

Instead of using delete, replace the element to be removed with the last element in the array and then reduce the array's length. This approach maintains a contiguous array without gaps.

function transferReward(address _to, uint256 _index) public {
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
rewardsOwned[_to].push(rewardsOwned[msg.sender][_index]);
- delete rewardsOwned[msg.sender][_index];
// Swap the element to be deleted with the last element
+ uint256 lastIndex = rewardsOwned[msg.sender].length - 1;
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][lastIndex];
+ rewardsOwned[msg.sender].pop(); // Remove the last element
}
Updates

Appeal created

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

A user can poison the `rewardsOwned` of another user via `transferReward` of an empty reward index

Support

FAQs

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

Give us feedback!