Summary
In the Pot:closePot function, the claimantCut variable having an incorrect calculation, resulting players who claim their rewards on time receiving less than the expected amount of additional rewards. The discrepancy is due to the wrong denominator is used in the claimantCut.
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L57
Vulnerability Details
The claimantCut variable under the Pot:closePot function is used to calculate the additional rewards to distribute fairly among players who claim their rewards before the pot is closed. The current calculation does not properly account for the number of claimants. The calculation wrongly uses the i_players.length as the denominator to divide the remaining rewards among all players instead of dividing the remaining rewards to those players who make the claim on time.
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);
}
}
}
Proof of Concept:
In test/TestMyCut.t.sol, add the following test case:
function test_audit_incorrectClaimantCut() public mintAndApproveTokens {
uint256[] memory rewardsAdj = new uint256[](2);
rewardsAdj[0] = 3 ether;
rewardsAdj[1] = 1 ether;
uint256 totalRewardsAdj = 4 ether;
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewardsAdj, IERC20(weth), totalRewardsAdj);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
vm.prank(player1);
Pot(contest).claimCut();
vm.prank(user);
vm.warp(91 days);
ContestManager(conMan).closeContest(contest);
uint256 numerator = 1 ether * 90;
uint256 denominator = 100;
uint256 expectedPlayerBalance = 3 ether + (numerator / denominator);
uint256 actualPlayerBalance = weth.balanceOf(player1);
console.log("expectedPlayerBalance: ", expectedPlayerBalance);
console.log("actualPlayerBalance: ", actualPlayerBalance);
assert(actualPlayerBalance < expectedPlayerBalance);
}
In the console, the following is displayed :
Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[PASS] test_audit_incorrectClaimantCut() (gas: 907900)
Logs:
expectedPlayerBalance: 3900000000000000000
actualPlayerBalance: 3450000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.09ms (241.88µs CPU time)
The test run passes indicating that the actual player balance is less than the expected player balance due to the wrong calculation in the claimantCut.
Impact
Players who claim on time will get lesser than what they entitle to from the unclaimed rewards pot, affecting also the reputation and credibility of protocol team
Tools Used
Manual review
Recommendations
To correct the calculation of the claimantCut by replacing the denominator from i_players.length to claimants.length
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;
+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
console.log("claimantCut: ", claimantCut);
for (uint256 i = 0; i < claimants.length; i++) {
// @audit-unsure: https://github.com/crytic/slither/wiki/Detector-Documentation/#calls-inside-a-loop
_transferReward(claimants[i], claimantCut);
}
}
}
