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

[M-03] Improper deletion of array elements in MysteryBox:claimSingleReward, deleted data is persisted increasing gas costs for players

0xnelli

Summary

The rewards of a user are tracked in the mapping MysteryBox::rewardsOwned, which maps an address to an array of MysteryBox::Reward. When a player claims a reward by calling the MysteryBox::claimSingleReward function, an index is specified in the function and this MysteryBox::Reward is deleted from the array.

However, there is no resizing or reordering of the array. Therefore, this deleted MysteryBox::Reward remains in the array and will unnecessarily bloat gas costs when the array is retrieved or iterated through, as in MysteryBox::claimAllRewards.

Vulnerability details

As can be seen in the MysteryBox::claimSingleReward function, a specific index of the Rewards array is deleted and there is no resizing or reordering of the array once this deletion has occurred.

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];
}

As a result, there will be gaps left in the array once a player claims their specific reward. These gaps in the array will be of type MysteryBox::Reward and have the values name: "" and value: 0. These empty reward structs remain in the same index in the array and will add to gas costs whenever it is retrieved or iterated through.

Adding the following code into the TestMysteryBox.t.sol file illustrates the preservation of deleted data in the rewards array of players:

modifier boxesBought() {
uint256 boxPrice = mysteryBox.boxPrice();
vm.startPrank(user1);
for (uint8 i = 0; i < 3; i++) {
mysteryBox.buyBox{value: boxPrice}();
vm.warp(4);
mysteryBox.openBox();
}
vm.stopPrank();
_;
}
function testClaimSingleReward_Gaps() public boxesBought {
vm.startPrank(user1);
mysteryBox.claimSingleReward(1);
​
MysteryBox.Reward[] memory rewards = mysteryBox.getRewards();
vm.stopPrank();
for (uint8 i = 0; i < rewards.length; i++) {
console2.log("Reward name: %s, reward value: %s", rewards[i].name, rewards[i].value);
}
assertEq(rewards[1].name, "");
assertEq(rewards[1].value, 0);
}

The output of the console was the following:

Reward name: Bronze Coin, reward value: 100000000000000000
Reward name: , reward value: 0
Reward name: Bronze Coin, reward value: 100000000000000000

This verifies that the reward at index 1 has been claimed, but an empty reward struct remains in its place.

Impact

This will increase gas costs for users whenever the rewards array of player has to be retrieved or iterated over.

Tools used

Manual review

Recommended Mitigation

In the MysteryBox::claimSingleReward function, instead of deleting the index of the reward that was claimed, the last element in the reward array can be moved to this index and the array can be resized to its current length less one.

This ensures that the redundant data is deleted and only non-claimed rewards are stored. Additionally, if the _index of the reward that is being claimed is in fact the last element we can just immediately resize the rewards array.

This logic is shown below:

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 claimed reward logic
uint256 lastElementIndex = rewardsOwned[msg.sender].length - 1;
if (_index != lastElementIndex) {
rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][lastElementIndex];
}
rewardsOwned[msg.sender].pop();
}
Updates

Appeal created

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!