Summary
Bad division inside closePot function causes "claimants" to get less tokens than expected (if there are players who not claimed, or same player address added more times) and there still will be tokens stucked inside protocol even when pot is closed.
Vulnerability Details
In closePot function, there is calculation for claimant cut, which is calculated by subtracting manager cut from remaining rewards and then dividing by length of players array. However, there are all players inside players array, not only players who claimed. Because of that, claimants get less tokens than deserve and there will be tokens inside protocol even after closing the pot.
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
Claimants get less tokens than expected. Protocol still holds tokens after pot is closed.
Because of remaining balance of tokens inside the protocol, there is possible to call closePot more times and distribute tokens to manager and claimants again.
PoC
Add following function to the TestMyCut.t.sol test file:
function testClaimantsGetLessTokens() public mintAndApproveTokens {
address player3 = makeAddr("player3");
address player4 = makeAddr("player4");
address player5 = makeAddr("player5");
players = [player1, player2, player1, player3, player4, player5];
rewards = [500, 100, 1, 20, 500, 600];
totalRewards = 100000;
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
address[] memory claimants = new address[](2);
claimants[0] = player1;
claimants[1] = player4;
for (uint256 i = 0; i < claimants.length; i++) {
vm.prank(claimants[i]);
Pot(contest).claimCut();
}
uint256 remainingRewards = Pot(contest).getRemainingRewards();
uint256 managerCut = remainingRewards / 10;
uint256 claimantCutWrong = (remainingRewards - managerCut) / players.length;
uint256 claimantCutOk = (remainingRewards - managerCut) / claimants.length;
console.log("Pot balance after claims:", weth.balanceOf(address(Pot(contest))));
console.log("Pot remaining rewards after claims:", remainingRewards);
uint256 potBalance = weth.balanceOf(address(Pot(contest))) - managerCut;
uint256 remainingOk = potBalance - claimantCutOk * 2;
assertEq(remainingOk, 0);
uint256 claimantBalanceBefore = weth.balanceOf(address(player4));
vm.warp(91 days);
vm.prank(user);
ContestManager(conMan).closeContest(contest);
potBalance = weth.balanceOf(address(Pot(contest)));
uint256 claimantBalance = weth.balanceOf(address(player4));
assertGt(potBalance, 0);
assertEq(claimantCutWrong, claimantBalance - claimantBalanceBefore);
assertLt(claimantBalance, claimantCutOk);
console.log("Pot balance after first close:", potBalance);
console.log("Pot remaining rewards after first close:", Pot(contest).getRemainingRewards());
vm.prank(user);
ContestManager(conMan).closeContest(contest);
assertGt(weth.balanceOf(address(Pot(contest))), 0);
console.log("Pot balance after second close:", weth.balanceOf(address(Pot(contest))));
console.log("Pot remaining rewards after second close:", Pot(contest).getRemainingRewards());
}
and then pass following to the command line forge test --mt testClaimantsGetLessTokens -vvv to run test.
Tools Used
Manual review.
Recommendations
Swap i_players.length with claimants.length when calculating claimants cut.
Another thing to reccomend is to subtract sended tokens from remainingRewards.
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
++ uint256 rewardsSended = managerCut;
i_token.transfer(msg.sender, managerCut);
-- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
++ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
++ rewardsSended += claimantCut;
}
++ remainingRewards -= rewardsSended;
}
}
