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

[M-02] Improper deletion of array elements in MysteryBox::transferReward, deleted data is persisted increasing gas costs for users

0xnelli

Summary

The rewards of a user are tracked in a mapping of MysteryBox::rewardsOwned which maps an address to an array of MysteryBox::Reward. When a user transfers a reward to another address by calling the MysteryBox::transferReward function, an index is specified in the function and this MysteryBox::Reward is deleted from the array. However, there is no resizing of the array. Therefore this deleted MysteryBox::Reward remains in the array and will unecessarily bloat gas costs when retrieving the array, as in MysteryBox::claimAllRewards.

Vulnerability Details

In the MysteryBox:transferReward function below, a specific index of the rewardsOwned array is deleted and there is no resizing or reordering of the array once this deletion has occurred.

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

As a result, gaps will be left in the array once a player transfers their rewards to another address. These gaps 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 be retrieved whenever the RewardsOwned is used. One such example is in the MysteryBox::claimAllRewards function.

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}();
mysteryBox.openBox();
}
vm.stopPrank();
_;
}
​
function testTransferReward_Gaps() public boxesBought {
vm.startPrank(user1);
mysteryBox.transferReward(user2, 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 to the console was the following:

Reward name: Coal, reward value: 0
Reward name: , reward value: 0
Reward name: Coal, reward value: 0

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

Impact

Players will face increased gas costs for users whenever the rewardsOwned array is retrieved or iterated over, as in the MysteryBox::claimAllRewards function, as deleted useless data is preserved.

Tools Used

Manual review

Recommendations

In the MysteryBox::transferReward function, the last element can be moved into the index to be deleted and the array can be resized to be one element less than its current length. However, if the _index parameter is the last element then we can simply resize. This is demonstrated below:

function transferReward(address _to, uint256 _index) public {
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
rewardsOwned[_to].push(rewardsOwned[msg.sender][_index]);
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
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!