Submission Details
Severity:
getRewards shows 1 empty reward after transferReward
Summary
If a user transfer has reward to someone else he still has one empty reward which should not be the case.
Vulnerability Details
If a user doesn't have any reward and calls getRewards function, it return nothing and the length of the rewards is zero. This is good, as in the output given below.
function testGetRewards_NoReward() public {
vm.deal(user1, 0.1 ether);
vm.prank(user1);
console.log("Rewards : ", mysteryBox.getRewards().length);
assertEq(mysteryBox.getRewards().length, 0);
}
[PASS] testGetRewards_NoReward() (gas: 21230)
Logs:
Rewards : 0
Traces:
[21230] MysteryBoxTest::testGetRewards_NoReward()
├─ [0] VM::deal(0x0000000000000000000000000000000000000001, 100000000000000000 [1e17])
│ └─ ← [Return]
├─ [0] VM::prank(0x0000000000000000000000000000000000000001)
│ └─ ← [Return]
├─ [2688] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] []
├─ [0] console::log("Rewards : ", 0) [staticcall]
│ └─ ← [Stop]
├─ [2688] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] []
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
But when a user has 1 reward and he transfers it to someone else and he should have zero reward but getRewards return an empty rewards with length 1. `Reward({ name: "", value: 0 }` as show in the example below.
function testGetRewards_AfterTransferReward() public {
vm.deal(user1, 0.1 ether);
vm.startPrank(user2);
console.log("User2 Rewards Before Transfer: ", mysteryBox.getRewards().length);
vm.startPrank(user1);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
// assertEq(mysteryBox.getRewards().length, 1);
console.log("User1 Rewards Before Transfer: ", mysteryBox.getRewards().length);
mysteryBox.transferReward(user2, 0);
console.log("User1 Rewards After Transfer: ", mysteryBox.getRewards().length);
// assertEq(mysteryBox.getRewards().length, 0);
vm.stopPrank();
vm.prank(user2);
console.log("User2 Rewards Before Transfer: ", mysteryBox.getRewards().length);
// assertEq(mysteryBox.getRewards().length, 0);
}
[PASS] testGetRewards_AfterTransferReward() (gas: 126308)
Logs:
User2 Rewards Before Transfer: 0
User1 Rewards Before Transfer: 1
User1 Rewards After Transfer: 1
User2 Rewards Before Transfer: 1
Traces:
[130520] MysteryBoxTest::testGetRewards_AfterTransferReward()
├─ [0] VM::deal(0x0000000000000000000000000000000000000001, 100000000000000000 [1e17])
│ └─ ← [Return]
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000002)
│ └─ ← [Return]
├─ [2688] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] []
├─ [0] console::log("User2 Rewards Before Transfer: ", 0) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001)
│ └─ ← [Return]
├─ [24580] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [49209] MysteryBox::openBox()
│ ├─ emit RandomValueGenerated(randomValue: 15)
│ └─ ← [Stop]
├─ [2130] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] [Reward({ name: "Coal", value: 0 })]
├─ [0] console::log("User1 Rewards Before Transfer: ", 1) [staticcall]
│ └─ ← [Stop]
├─ [47050] MysteryBox::transferReward(0x0000000000000000000000000000000000000002, 0)
│ └─ ← [Stop]
├─ [1875] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] [Reward({ name: "", value: 0 })]
├─ [0] console::log("User1 Rewards After Transfer: ", 1) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::prank(0x0000000000000000000000000000000000000002)
│ └─ ← [Return]
├─ [2130] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] [Reward({ name: "Coal", value: 0 })]
├─ [0] console::log("User2 Rewards Before Transfer: ", 1) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Impact
This can cause invalid state.
Tools Used
Recommendations
if there are no rewards in name of a user then getRewards function should return empty list with zero length but not the empty reward.
The transfer reward function should be modified as below:
function transferReward(address _to, uint256 _index) public {
// require(_to != address(0), "Invalid address");
require(_index < rewardsOwned[msg.sender].length, "Invalid index");
rewardsOwned[_to].push(rewardsOwned[msg.sender][_index]);
+ rewardsOwned[msg.sender][_index] = rewardsOwned[msg.sender][rewardsOwned[msg.sender].length - 1];
+ rewardsOwned[msg.sender].pop();
- // delete rewardsOwned[msg.sender][_index];
}
Updates
Appeal created
inallhonesty 11 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.