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

The 'claimAllRewards' function claim insufficient,Cause revert: Transfer failed

mengwp2004

Summary

If user's reward is great than contract balance, Use 'claimAllRewards' function claim insufficient will cause asset transfer failed.

Vulnerability Details

User open mystery box may be get large reward.If user's reward is great than contract balance,User cann't claim assert.

Impact

proof of concepts

function testClaimAllRewardsInsufficient() public {
​
address user3 = address(0x4);
vm.deal(user1, 1 ether);
vm.startPrank(user1);
mysteryBox.buyBox{value: 0.1 ether}();
vm.stopPrank();
​
vm.deal(user3, 1 ether);
vm.startPrank(user3);
mysteryBox.buyBox{value: 0.1 ether}();
console.log("User Buy box");
mysteryBox.openBox();
console.log("User Open box");
assertEq(mysteryBox.boxesOwned(user3), 0);
​
MysteryBox.Reward[] memory rewards = mysteryBox.getRewards();
console.log("User rewards name:",rewards[0].name);
console.log("User rewards value:",rewards[0].value);
uint256 balance = address(mysteryBox).balance;
console.log("Contract balance ",balance);
console.log("User reward balance is greater than contract balance:",rewards[0].value > balance);
mysteryBox.claimAllRewards();
vm.stopPrank();
}

forge test --match-test testClaimAllRewardsInsufficient -vvv
[⠊] Compiling...
[⠰] Compiling 1 files with Solc 0.8.21
[⠔] Solc 0.8.21 finished in 1.39s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
--> test/TestMysteryBox.t.sol:267:5:
|
267 | function Attack() public {
| ^ (Relevant source part starts here and spans across multiple lines).

Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[FAIL. Reason: revert: Transfer failed] testClaimAllRewardsInsufficient() (gas: 163666)
Logs:
Reward Pool Length: 4
User Buy box
User Open box
User rewards name: Silver Coin
User rewards value: 500000000000000000
Contract balance 200000000000000000
User reward balance is greater than contract balance: true

Traces:
[163666] MysteryBoxTest::testClaimAllRewardsInsufficient()
├─ [0] VM::deal(ECRecover: [0x0000000000000000000000000000000000000001], 1000000000000000000 [1e18])
│ └─ ← [Return]
├─ [0] VM::startPrank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [24580] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::deal(Identity: [0x0000000000000000000000000000000000000004], 1000000000000000000 [1e18])
│ └─ ← [Return]
├─ [0] VM::startPrank(Identity: [0x0000000000000000000000000000000000000004])
│ └─ ← [Return]
├─ [22580] MysteryBox::buyBox{value: 100000000000000000}()
│ └─ ← [Stop]
├─ [0] console::log("User Buy box") [staticcall]
│ └─ ← [Stop]
├─ [68092] MysteryBox::openBox()
│ └─ ← [Stop]
├─ [0] console::log("User Open box") [staticcall]
│ └─ ← [Stop]
├─ [608] MysteryBox::boxesOwned(Identity: [0x0000000000000000000000000000000000000004]) [staticcall]
│ └─ ← [Return] 0
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [2130] MysteryBox::getRewards() [staticcall]
│ └─ ← [Return] [Reward({ name: "Silver Coin", value: 500000000000000000 [5e17] })]
├─ [0] console::log("User rewards name:", "Silver Coin") [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("User rewards value:", 500000000000000000 [5e17]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Contract balance ", 200000000000000000 [2e17]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("User reward balance is greater than contract balance:", true) [staticcall]
│ └─ ← [Stop]
├─ [8198] MysteryBox::claimAllRewards()
│ ├─ [0] PRECOMPILES::identity{value: 500000000000000000}(0x)
│ │ └─ ← [OutOfFunds]
│ └─ ← [Revert] revert: Transfer failed
└─ ← [Revert] revert: Transfer failed

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.41ms (1.21ms CPU time)

Ran 1 test suite in 196.20ms (9.41ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:

Tools Used

Manual review.

Recommendations

If user's reward is great than contract balance,user can claim all of contract balance.

function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
​
+ if(totalValue > address(this).balance){
+ totalValue = address(this).balance;
+ }
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
​
delete rewardsOwned[msg.sender];
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago

Appeal created

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

Protocol should have a higher initial balance to prevent prize withdrawing problems

Support

FAQs

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

Give us feedback!