githublink
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L54
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L57
Description:
1.The smart contract function for distributing rewards among claimants uses i_players.length as the divisor to calculate the claimantCut. However, if i_players.length is greater than claimants.length, it may result in an over-allocation of rewards per claimant, leading to unclaimed funds and potential wastage of rewards.
2.If no one claims the rewards, then the loop
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
will not distribute the rewards, resulting in unclaimed funds that could be wasted. It is crucial to ensure that there is a mechanism to handle such scenarios to avoid the wastage of allocated rewards.
3.Because players can choose not to call the claimCut() function, which includes the check if (reward <= 0), this condition will not be verified. Therefore, if there are many participants with rewards set to zero that remain unclaimed, it can easily lead to a situation where the number of people exceeds the available funds, resulting in some rewards not being fully claimed and ultimately not all rewards being distributed. Because of uint256 claimantCut downward rounding, the rewards will not be collected, and what is distributed to others will be zero.
4.he variables remainingRewards, managerCutPercent, and managerCut are all of the uint256 type. When performing division operations with uint256 types, the result of managerCut automatically discards the fractional part and rounds down to the nearest whole number.
Impact:
Inefficient Reward Distribution: Using i_players.length instead of claimants.length as the divisor can lead to each claimant receiving more than their rightful share, resulting in some rewards remaining unclaimed.Since managerCutPercent represents the percentage of the reward amount allocated to the manager, if the result of remainingRewards / managerCutPercent is not an integer, the precise manager's cut will not be achieved.
Unclaimed Rewards: If no claimants come forward to receive their rewards, the loop designed to distribute the rewards will not execute, leaving the intended rewards unclaimed.
An attack involving a large population of participants with zero rewards.if there are many participants with rewards set to zero that remain unclaimed, it can easily lead to a situation where the number of people exceeds the available funds, resulting in some rewards not being fully claimed and ultimately not all rewards being distributed. Because of uint256 claimantCut downward rounding, the rewards will not be collected, and what is distributed to others will be zero.
Proof of Concept:
Use Foundry Tools and Test File:
Tool Used: Foundry (forge 0.2.0)
Test File: TestMyCut.t.sol
Issue 1: Inefficient Reward Distribution for claimant
pragma solidity ^0.8.20;
import {ContestManager} from "../src/ContestManager.sol";
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {Pot} from "../src/Pot.sol";
contract TestMyCut is Test {
address conMan;
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2];
uint256 public constant STARTING_USER_BALANCE = 1000 ether;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [5, 10];
address user = makeAddr("user");
uint256 totalRewards = 15;
function setUp() public {
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
console.log("User Address: ", user);
console.log("Contest Manager Address 1: ", address(conMan));
vm.stopPrank();
}
modifier mintAndApproveTokens() {
console.log("Minting tokens to: ", user);
vm.startPrank(user);
ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
ERC20Mock(weth).approve(conMan, STARTING_USER_BALANCE);
console.log("Approved tokens to: ", address(conMan));
vm.stopPrank();
_;
}
function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
uint256 poolBalanceAfter = ERC20Mock(weth).balanceOf(contest);
uint256 claimantBalanceafter = ERC20Mock(weth).balanceOf(player1);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(user);
assert(poolBalanceAfter ==0);
}
}
├─ [641] ERC20Mock::balanceOf(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]) [staticcall]
│ └─ ← [Return] 5
├─ [641] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ └─ ← [Return] 9
├─ [641] ERC20Mock::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← [Return] 999999999999999999985 [9.999e20]
└─ ← [Revert] panic: assertion failed (0x01)
Originally, it was expected that the distribution would result in player1 receiving 14 tokens, with the Pot contract having a remaining balance of 0. However, the actual outcome is as follows:
pragma solidity ^0.8.20;
import {ContestManager} from "../src/ContestManager.sol";
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {Pot} from "../src/Pot.sol";
contract TestMyCut is Test {
address conMan;
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2];
uint256 public constant STARTING_USER_BALANCE = 1000 ether;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [6, 10];
address user = makeAddr("user");
uint256 totalRewards = 15;
function setUp() public {
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
console.log("User Address: ", user);
console.log("Contest Manager Address 1: ", address(conMan));
vm.stopPrank();
}
modifier mintAndApproveTokens() {
console.log("Minting tokens to: ", user);
vm.startPrank(user);
ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
ERC20Mock(weth).approve(conMan, STARTING_USER_BALANCE);
console.log("Approved tokens to: ", address(conMan));
vm.stopPrank();
_;
}
function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
uint256 poolBalanceAfter = ERC20Mock(weth).balanceOf(contest);
uint256 claimantBalanceafter = ERC20Mock(weth).balanceOf(player1);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(conMan);
assert(poolBalanceAfter ==0);
}
}
├─ [641] ERC20Mock::balanceOf(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]) [staticcall]
│ └─ ← [Return] 5
├─ [641] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ └─ ← [Return] 10
├─ [641] ERC20Mock::balanceOf(conMan: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← [Return] 0
└─ ← [Revert] panic: assertion failed (0x01)
Because when 9 is divided by 10 and rounded down, the result is 0. Therefore, in this case, the conMan, or the manager, does not receive a single cent, which is such an unfair distribution system that leads to a loss.
Issue 3: Unclaimed Rewards Due to Lack of Invocation
pragma solidity ^0.8.20;
import {ContestManager} from "../src/ContestManager.sol";
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {Pot} from "../src/Pot.sol";
contract TestMyCut is Test {
address conMan;
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2];
uint256 public constant STARTING_USER_BALANCE = 1000 ether;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [5, 10];
address user = makeAddr("user");
uint256 totalRewards = 15;
function setUp() public {
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
console.log("User Address: ", user);
console.log("Contest Manager Address 1: ", address(conMan));
vm.stopPrank();
}
modifier mintAndApproveTokens() {
console.log("Minting tokens to: ", user);
vm.startPrank(user);
ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
ERC20Mock(weth).approve(conMan, STARTING_USER_BALANCE);
console.log("Approved tokens to: ", address(conMan));
vm.stopPrank();
_;
}
function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
uint256 poolBalanceAfter = ERC20Mock(weth).balanceOf(contest);
uint256 claimantBalanceafter = ERC20Mock(weth).balanceOf(player1);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(user);
assert(poolBalanceAfter ==0);
}
}
├─ [641] ERC20Mock::balanceOf(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]) [staticcall]
│ └─ ← [Return] 14
├─ [641] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ └─ ← [Return] 0
├─ [641] ERC20Mock::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← [Return] 999999999999999999985 [9.999e20]
If no one claims the rewards, then the distribution loop is not entered, resulting in a significant loss of funds. In this case, the expected balance in the Pot contract should be 0, but it is actually 14, indicating that these funds have not been claimed and are effectively lost：
**Issue4： An attack involving a large population of participants with zero rewards.
When the remaining rewards are divided by the number of players (i_players.length), the calculation uint256 claimantCut = (remainingRewards - managerCut) / i_players.length：
pragma solidity ^0.8.20;
import {ContestManager} from "../src/ContestManager.sol";
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {Strings} from "lib/openzeppelin-contracts/contracts/utils/Strings.sol";
import {Pot} from "../src/Pot.sol";
contract TestMyCut is Test {
address conMan;
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2];
uint256 public constant STARTING_USER_BALANCE = 1000 ether;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [5, 10];
address user = makeAddr("user");
uint256 totalRewards = 15;
function setUp() public {
for (uint256 i = 3; i < 13; i++) {
string memory playerName = string(abi.encodePacked("player", Strings.toString(i)));
address playerAddr = makeAddr(playerName);
players.push(playerAddr);
rewards.push(uint256(0));
}
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
console.log("User Address: ", user);
console.log("Contest Manager Address 1: ", address(conMan));
vm.stopPrank();
}
modifier mintAndApproveTokens() {
console.log("Minting tokens to: ", user);
vm.startPrank(user);
ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
ERC20Mock(weth).approve(conMan, STARTING_USER_BALANCE);
console.log("Approved tokens to: ", address(conMan));
vm.stopPrank();
_;
}
function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
uint256 poolBalanceAfter = ERC20Mock(weth).balanceOf(contest);
uint256 claimantBalanceafter = ERC20Mock(weth).balanceOf(player1);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(user);
assert(poolBalanceAfter ==0);
}
}
ERC20Mock::balanceOf(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]) [staticcall]
← [Return] 9 (Unclaimed rewards remaining in the Pot)
ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
← [Return] 5 (Amount received by player1, expected was 14)
ERC20Mock::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
← [Return] 999999999999999999985 (Balance of the user, nearly the full initial balance)
results in zero due to the truncation of the decimal part. This leads to a situation where no participants receive their deserved rewards, even if there are sufficient funds in the Pot.The expected balance in the Pot should be 0, and player1 should have received 14 tokens. However, due to the integer division truncation, player1 ended up receiving only 5 tokens, and in subsequent distributions, no further rewards were allocated to player1.
Recommended Mitigation:
- uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
- for (uint256 i = 0; i < claimants.length; i++) {
- _transferReward(claimants[i], claimantCut);
- }
+ // Calculate the total distributable rewards and initialize variables for distribution
+ uint256 totalDistributableRewards = remainingRewards - managerCut;
+ uint256 basicReward = totalDistributableRewards / claimants.length;
+ uint256 remainder = totalDistributableRewards % claimants.length;
+
+ // Distribute the rewards
+ for (uint256 i = 0; i < claimants.length; i++) {
+ uint256 rewardForClaimant = basicReward;
+ if (remainder > 0) {
+ rewardForClaimant += 1; // Distribute one extra unit to the current claimant
+ remainder--; // Decrease the remainder by one
+ }
+ _transferReward(claimants[i], rewardForClaimant);
+ }
+ // Ensure all rewards are distributed by the end of the loop
+ if (remainder>0){
+ _transferReward(msg.sender, remainder)
+//if nobody claim the rewards,the remainrewards will all send to manager
+}
Benefits of this modification include:
-
Fair Distribution: By calculating the remainder and distributing it one by one to the claimants, each participant is ensured to receive their fair share, including any extra amounts that result from division.
-
Performance Efficiency: Minimizing the operations performed in the loop reduces the gas cost and execution time, which is particularly beneficial when dealing with a large number of claimants.
-
Accuracy: This method prevents the loss of funds that can occur due to integer division by ensuring that every unit of the reward is accounted for and distributed.
-
Simplicity: The code is made simpler and more readable by reducing the complexity within the loop and by clearly separating the calculation of the basic reward and the handling of the remainder.
-
Robustness: The use of an assertion at the end of the distribution process adds a layer of security, ensuring that the contract behaves as expected and all funds are allocated correctly.
