Submission Details
Severity:
Missing input check in `Pot` constructor for `totalRewards` can break the protocol
Summary
Potcontract constructor lacks the checking for totalRewards is equals to the sum of rewards, Any mismatch can cause some users not be able to claim rewards.
Vulnerability Details
https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L22-L35
constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
i_players = players;
i_rewards = rewards;
i_token = token;
i_totalRewards = totalRewards;
remainingRewards = totalRewards;
i_deployedAt = block.timestamp;
// i_token.transfer(address(this), i_totalRewards);
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}
If we check above snippet, there is no check for totalRewards is equals to the sum of rewards or not. This lack of input validation can cause issue in claimCutand closePotin different scenerios.
-
Scenerio 1 -
When users amounts are 50, 40, 30 and total rewards is set 30. if within given duration only first user try to claim, it will revert due to condition
remainingRewards -= reward;in claimCut, due to the curernt values 30 - 50 -
Scenerio 2 -
When total rewards is set higher than sum of user amounts in that case, claim will work fine, but it will loss to the protocol, as extra tokens will sent to the participants.
As there is no way rectify the owner mistake, if he accidently put wrong number.
POC
Add following test in existing test suite
case1: when total rewards are more than required.
function testTotalRewardsMismatch() public mintAndApproveTokens {
// Setup
address[] memory testPlayers = new address[](3);
testPlayers[0] = makeAddr("player1");
testPlayers[1] = makeAddr("player2");
testPlayers[2] = makeAddr("player3");
uint256[] memory testRewards = new uint256[](3);
testRewards[0] = 400;
testRewards[1] = 300;
testRewards[2] = 300;
uint256 incorrectTotalRewards = 1200; // Actual sum is 1000
// Create contest with mismatched total rewards
vm.startPrank(user);
address testContest = ContestManager(conMan).createContest(testPlayers, testRewards, IERC20(ERC20Mock(weth)), incorrectTotalRewards);
vm.stopPrank();
// Verify that the contest was created despite the mismatch
address[] memory contests = ContestManager(conMan).getContests();
assertEq(contests[contests.length - 1], testContest, "Contest should be created despite total rewards mismatch");
// Fund the contest
vm.startPrank(user);
ContestManager(conMan).fundContest(ContestManager(conMan).getContests().length - 1);
vm.stopPrank();
// Verify the funded amount
uint256 contestBalance = ERC20Mock(weth).balanceOf(testContest);
assertEq(contestBalance, incorrectTotalRewards, "Contest should be funded with the incorrect total rewards");
// Let players claim their rewards
for (uint256 i = 0; i < testPlayers.length; i++) {
vm.prank(testPlayers[i]);
Pot(testContest).claimCut();
}
uint256 user1Balance = ERC20Mock(weth).balanceOf(testPlayers[0]);
uint256 user2Balance = ERC20Mock(weth).balanceOf(testPlayers[1]);
uint256 user3Balance = ERC20Mock(weth).balanceOf(testPlayers[2]);
console.log("user1 balance after cut:", user1Balance);
console.log("user2 balance after cut:", user2Balance);
console.log("user3 balance after cut:", user3Balance);
// Verify the remaining balance
uint256 remainingBalance = ERC20Mock(weth).balanceOf(testContest);
assertEq(remainingBalance, incorrectTotalRewards - 1000, "Excess funds should remain in the contract");
// Fast forward 91 days
vm.warp(block.timestamp + 91 days);
uint256 remaining = ERC20Mock(weth).balanceOf(testContest);
// Close the contest
vm.prank(user);
ContestManager(conMan).closeContest(testContest);
// Verify final balances
uint256 finalContestBalance = ERC20Mock(weth).balanceOf(testContest);
/// owner gonna get 10% of remaining tokens (i.e 20 tokens)
uint256 ownerBalance = ERC20Mock(weth).balanceOf(address(conMan));
console.log("ownerBalance:", ownerBalance);
/// rest funds, split among 3 users equally which is loss to the protocol
/// ideally, it should have refund to owner as all users have claimed
user1Balance = ERC20Mock(weth).balanceOf(testPlayers[0]);
user2Balance = ERC20Mock(weth).balanceOf(testPlayers[1]);
user3Balance = ERC20Mock(weth).balanceOf(testPlayers[2]);
console.log("user1 balance after contest ends:", user1Balance);
console.log("user2 balance after contest ends:", user2Balance);
console.log("user3 balance after contest ends:", user3Balance);
uint256 protocolLoss = remaining - ownerBalance;
assertEq(finalContestBalance, 0, "All funds should be distributed");
assertEq(ownerBalance, 20, "Owner should receive cut from excess funds");
console.log("protocol loss:",protocolLoss);
}
run forge test --mt testTotalRewardsMismatchin the terminal, following output will be shown:
[⠊] Compiling...
[⠑] Compiling 1 files with Solc 0.8.20
[⠃] Solc 0.8.20 finished in 1.74s
Compiler run successful!
Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[PASS] testTotalRewardsMismatch() (gas: 977651)
Logs:
User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
user1 balance after cut: 400
user2 balance after cut: 300
user3 balance after cut: 300
ownerBalance: 20
user1 balance after contest ends: 460
user2 balance after contest ends: 360
user3 balance after contest ends: 360
protocol loss: 180
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.80ms (669.92µs CPU time)
case2: when total rewards are less than required:
Add the following test in existing test suite:
function testTotalRewardsMismatchClaimRevert() public mintAndApproveTokens {
// Setup
address[] memory testPlayers = new address[](3);
testPlayers[0] = makeAddr("player1");
testPlayers[1] = makeAddr("player2");
testPlayers[2] = makeAddr("player3");
uint256[] memory testRewards = new uint256[](3);
testRewards[0] = 100;
testRewards[1] = 200;
testRewards[2] = 500;
uint256 incorrectTotalRewards = 500; // Actual sum is 800
// Create contest with mismatched total rewards
vm.startPrank(user);
address testContest = ContestManager(conMan).createContest(testPlayers, testRewards, IERC20(ERC20Mock(weth)), incorrectTotalRewards);
vm.stopPrank();
// Verify that the contest was created despite the mismatch
address[] memory contests = ContestManager(conMan).getContests();
assertEq(contests[contests.length - 1], testContest, "Contest should be created despite total rewards mismatch");
// Fund the contest
vm.startPrank(user);
ContestManager(conMan).fundContest(ContestManager(conMan).getContests().length - 1);
vm.stopPrank();
// Verify the funded amount
uint256 contestBalance = ERC20Mock(weth).balanceOf(testContest);
assertEq(contestBalance, incorrectTotalRewards, "Contest should be funded with the incorrect total rewards");
// Let players claim their rewards
// will revert due to underflow, when third user gonna try
// as lack of funds
for (uint256 i = 0; i < testPlayers.length; i++) {
vm.prank(testPlayers[i]);
Pot(testContest).claimCut();
}
}
when running forge test --mt testTotalRewardsMismatchClaimRevert -vv it will show following output:
[⠊] Compiling...
[⠘] Compiling 1 files with Solc 0.8.20
[⠃] Solc 0.8.20 finished in 1.74s
Compiler run successful!
Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testTotalRewardsMismatchClaimRevert() (gas: 946120)
Logs:
User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.73ms (559.67µs CPU time)
Ran 1 test suite in 160.49ms (1.73ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/TestMyCut.t.sol:TestMyCut
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testTotalRewardsMismatchClaimRevert() (gas: 946120)
Impact
Protocol could break potentially don't allow claims when total pools is less than sum of amounts and locked funds in the pool, when totalRewards are more than sum of rewards, extra funds will split among claimaints / participants causing loss to the protocol.
Tools Used
Manual Review, Foundry
Recommendations
Add a check to make sure, totalRewards must be equal to sum of rewards.
constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
i_players = players;
i_rewards = rewards;
i_token = token;
i_totalRewards = totalRewards;
remainingRewards = totalRewards;
i_deployedAt = block.timestamp;
uint256 sumRewards = 0;
for (uint256 i = 0; i < rewards.length; i++) {
sumRewards += rewards[i];
}
require(sumRewards == totalRewards, "Sum of rewards must equal total rewards");
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}
Rewards breakdown
nSLOC
106This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.