MyCut
First Flight #23
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

[H-01] Improper validation of `totalRewards` variable in `ContestManager::createContest` will lead to Denial of Service via underflow when a player tries to claim rewards.

madhuvarun

[H-01] Improper validation of totalRewards variable in ContestManager::createContest will lead to Denial of Service via underflow when a player tries to claim rewards.

Description: Contest manager can create a contest via ContestManager::createContest function. It takes in among other arguments rewards array (it contains reward for each player) and a totalRewards (the sum of all players' rewards). When the contest manager accidentally sends totalRewards less than the sum of rewards array it will lead to denial of service when a player tries to claim rewards using Pot::claimCut which will throw an error because remainingRewards would underflow.

Vulnerability Analysis: The vulnerability occurs at https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L44

Imagine a scenario. The contest manager creates a contest with 2 players whose addresses are [a1, a2] and their corresponding rewards are [5, 12]. The sum of rewards is 17. The contest manager accidentally set totalRewards to 13 in ContestManager::createContest. The pot is now deployed and is online for players to interact with. Let's say players will start claiming their rewards in the following way.

When address a2 claims their rewards using Pot::claimCut, players reward reward is subtracted from remainingRewards which would be 13 - 12 = 1

Now when address a1 tries to claim rewards, since remaningRewards is 1 which is less than players reward 5, performing remainingRewards -= reward; would underflow which results in a revert because remainingRewards is a uint. This underflow exception prevents a player from claiming their rewards.

Impact: Players won't be able to claim rewards assigned to them. Since players can't withdraw rewards. They cannot become claimants. This means that players loose their funds breaking the core functionality of the protocol.

Proof of code:

Paste the below code in test/TestMyCut.t.sol

​
function testUnderflow() public mintAndApproveTokens {
ContestManager cm = ContestManager(conMan);
uint playersLength = 2;
address[] memory p = new address[](playersLength);
uint256[] memory r = new uint256[](playersLength);
uint tr = 13;
​
p[0] = makeAddr("_player1");
p[1] = makeAddr("_player2");
r[0] = 5;
r[1] = 12;
​
vm.startPrank(user);
address pot = cm.createContest(p, r, weth, tr);
cm.fundContest(0);
vm.stopPrank();
​
vm.prank(p[1]); // player 2
Pot(pot).claimCut();
​
vm.prank(p[0]); // player 1
Pot(pot).claimCut();
}
​

Run the below test command in terminal

forge test --mt testUnderflow -vv

Which yields the below output

[⠒] Compiling...
[⠆] Compiling 1 files with 0.8.20
[⠔] Solc 0.8.20 finished in 2.64s
Compiler run successful!
​
Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testUnderflow() (gas: 813545)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.59ms (390.20µs CPU time)
​
Ran 1 test suite in 252.72ms (1.59ms 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)] testUnderflow() (gas: 813545)
​
Encountered a total of 1 failing tests, 0 tests succeeded
​

You will see that the test failed due to below reason

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testUnderflow()

Mitigation Recommendations: validate the totalRewards parameter to make sure that it matches the sum of rewards array. This can be done in ContestManager::createContest function as shown below

function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
public
onlyOwner
returns (address)
{
// Create a new Pot contract
+ uint _totalRewards;
+ for (uint i; i < rewards.length; ++i) {
+ _totalRewards += rewards[i];
+ }
+ if (_totalRewards != totalRewards) {
+ revert ContestManager__RewardsInvalid();
+ }
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}

The second way is to remove the totalRewards function argument and populate the value in the ContestManager::createContest function itself.

​
-function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards)
+function createContest(address[] memory players, uint256[] memory rewards, IERC20 token)
public
onlyOwner
returns (address)
{
// Create a new Pot contract
+ uint totalRewards;
+ for (uint i; i < rewards.length; ++i) {
+ totalRewards += rewards[i];
+ }
Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}
​

Tools used: Foundry, VSCode.

Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Appeal created

madhuvarun Submitter
about 1 year ago
equious Lead Judge
about 1 year ago
madhuvarun Submitter
about 1 year ago
equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.