Description:
The Pot::closePot function calculates the manager's cut as a percentage of the remaining funds in the pot. However, due to Solidity's truncation behavior, the calculated cut could be rounded down to zero if the resulting amount is small enough. This could unfairly deprive the manager of their rightful cut, particularly in scenarios where the remaining funds are minimal.
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
@> uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
Impact:
The manager could receive no compensation from the pot, undermining the financial incentives of managing the protocol. This is a critical issue as it directly affects the revenue model for managers.
Proof of Concept:
If the remaining funds in the pot are small, and the manager's cut (e.g., 10%) is calculated, the truncation during the division could lead to a result of zero, especially if the calculated cut is less than 1.
Add this to the TestMyCut.t.sol file.
function testManagerGetsUnfairZeroShare() public mintAndApproveTokens {
vm.startPrank(user);
rewards = [2, 2];
totalRewards = 4;
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 claimantBalanceAfter = ERC20Mock(weth).balanceOf(player1);
console.log("This is the claimantBalanceBefore::", claimantBalanceBefore);
console.log("This is the claimantBalanceAfter::", claimantBalanceAfter);
}
These are the following logs that were Outputted::
Traces:
[876492] TestMyCut::testManagerGetsUnfairZeroShare()
├─ [0] console::log("Minting tokens to: ", user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← [Return]
├─ [29712] ERC20Mock::mint(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 1000000000000000000000 [1e21])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], value: 1000000000000000000000 [1e21])
│ └─ ← [Stop]
├─ [24739] ERC20Mock::approve(ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], 1000000000000000000000 [1e21])
│ ├─ emit Approval(owner: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], spender: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], value: 1000000000000000000000 [1e21])
│ └─ ← [Return] true
├─ [0] console::log("Approved tokens to: ", ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← [Return]
├─ [634006] ContestManager::createContest([0x7026B763CBE7d4E72049EA67E89326432a50ef84, 0xEb0A3b7B96C1883858292F0039161abD287E3324], [2, 2], ERC20Mock: [0x5929B14F2984bBE5309c2eC9E7819060C31c970f], 4)
│ ├─ [530191] → new Pot@0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0
│ │ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353])
│ │ └─ ← [Return] 1513 bytes of code
│ └─ ← [Return] Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]
├─ [29108] ContestManager::fundContest(0)
│ ├─ [192] Pot::getToken() [staticcall]
│ │ └─ ← [Return] ERC20Mock: [0x5929B14F2984bBE5309c2eC9E7819060C31c970f]
│ ├─ [641] ERC20Mock::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ │ └─ ← [Return] 1000000000000000000000 [1e21]
│ ├─ [26084] ERC20Mock::transferFrom(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], 4)
│ │ ├─ emit Transfer(from: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], to: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], value: 4)
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84])
│ └─ ← [Return]
├─ [70849] Pot::claimCut()
│ ├─ [25188] ERC20Mock::transfer(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84], 2)
│ │ ├─ emit Transfer(from: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], to: player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84], value: 2)
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [641] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ └─ ← [Return] 2
├─ [0] VM::warp(7862400 [7.862e6])
│ └─ ← [Return]
├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← [Return]
├─ [12074] ContestManager::closeContest(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0])
│ ├─ [11219] Pot::closePot()
│ │ ├─ [5288] ERC20Mock::transfer(ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], 0)
│ │ │ ├─ emit Transfer(from: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], to: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], value: 0)
│ │ │ └─ ← [Return] true
│ │ ├─ [3288] ERC20Mock::transfer(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84], 1)
│ │ │ ├─ emit Transfer(from: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], to: player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84], value: 1)
│ │ │ └─ ← [Return] true
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [641] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ └─ ← [Return] 3
├─ [0] console::log("This is the claimantBalanceBefore::", 2) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("This is the claimantBalanceAfter::", 3) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
As you can see where we have the trace for the manager cut::
─ [12074] ContestManager::closeContest(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0])
│ ├─ [11219] Pot::closePot()
│ │ ├─ [5288] ERC20Mock::transfer(ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], 0)
│ │ │ ├─ emit Transfer(from: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], to: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], value: 0)
│ │ │ └─ ← [Return] true
The manager unfairly gets a cut of 0 mainly because of truncation. This could be found where we have the manager cut formula::
uint256 managerCut = remainingRewards / managerCutPercent;
Here the remainingRewards for this scenario was 2.
So 2/10 = 0.5.
In solidity, there is no handling of fractional arithmetic so it gets truncate to 0.
Recommended Mitigation:
To prevent truncation errors, consider implementing a minimum threshold for the manager's cut or using rounding-up techniques to ensure the manager always receives a non-zero amount when eligible.
