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

Vulnerability in Duplicate Address Handling in claimCut Function Leading to Unfair ETH Distribution.

archi

Summary

If a player is registered multiple times with different rewards, the player only receives the minimum of these rewards, with the remaining amount being redistributed. This results in a loss for the affected player while benefiting other players and the contest manager. Furthermore, if such a situation occurs, a player cannot claim their reward multiple times. Attempting to call claimCut twice by the same player results in a Pot__RewardNotFound() revert.

Vulnerability Details

The vulnerability primarily lies in the following line of code:

https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L39

Here, the claimCut function lacks a check for address duplication when assigning rewards to a player. As a result, in cases of duplicated addresses, the player receives only the minimum reward assigned to them.

Additionally, consider the following line of code:
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L43

In this line, the reward corresponding to a player is set to zero in the playersToRewards map. Consequently, if a player with a duplicated address has already claimed their reward once, any remaining rewards under their name are set to zero in the map. If they attempt to call the claimCut function again, the transaction reverts, as seen in the following if statement:

https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L40

Proof of Concept (PoC)

To demonstrate this vulnerability, I modified the players and rewards arrays as follows:

address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2, player1];
uint256[] rewards = [300, 100, 200];

Technically, player1 should receive 300+200=500ETH if claimed within 90 days, while player2 should receive 100 ETH.

However, to test the actual behavior, I created the following testDuplicacy function:

function testDuplicacy() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 6);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
vm.startPrank(player2);
Pot(contest).claimCut();
vm.stopPrank();
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
console.log("Contest Balance: ", ERC20Mock(weth).balanceOf(contest));
console.log("Player1 Balance: ", ERC20Mock(weth).balanceOf(player1));
console.log("Player2 Balance: ", ERC20Mock(weth).balanceOf(player2));
}

In this test, I attempted to call the claimCut function twice for player1, but it resulted in a revert:

Failing tests:
Encountered 1 failing test in test/TestMyCut.t.sol:TestMyCut
[FAIL. Reason: Pot__RewardNotFound()] testDuplicacy() (gas: 901491)

Next, I modified the testDuplicacy() function:

function testduplicacy() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 6);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
vm.startPrank(player2);
Pot(contest).claimCut();
vm.stopPrank();
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
​
console.log("Contest Balance: ", ERC20Mock(weth).balanceOf(conMan));
console.log("Player1 Balance: ", ERC20Mock(weth).balanceOf(player1));
console.log("Player2 Balance: ", ERC20Mock(weth).balanceOf(player2));
console.log("Contest Balance: ", ERC20Mock(weth).balanceOf(contest));
}

This resulted in the following output:

Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[PASS] testduplicacy() (gas: 958421)
Logs:
User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Contest Balance: 30
Player1 Balance: 290
Player2 Balance: 190
Contest Balance: 90

The test demonstrates that player1 is able to claim only once and therefore receives the minimum of the two rewards associated with it's address 200 ETH, while player2 receives their 100 ETH. The remaining 300 ETH is distributed among the players , ContestManagerand the contest contract as shown:-

Remaining Reward each player claimed their cut=300 ETH;
managerCut = remainingRewards / managerCutPercent=300/10=30 ETH;
claimantCut = (remainingRewards - managerCut) / players.length=(300-30)/3=90 ETH;
player1.balance+=90 ETH;
player2.balance+=90 ETH;
Rest of the 90 ETH remains in the contract;
Therefore, final balances:-
player1=290 ETH(500 ETH was assigned to this address);
player2=190 ETH(100 ETH was assigned to this address);
contest contract=90 ETH;
ContestManager=30 ETH;

Impact

  • Loss of ETH for player1

  • Gain in ETH for ContestManager and other players

  • Significant amount of ETH remains in the contest contract which neither goes to the players nor to the Contest Manager.

Tools Used

  • Manual Analysis

Recommendations

  • Implement a duplicate address check in the claimCut() function to prevent this vulnerability.

Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

incorrect handling of duplicate addresses

Support

FAQs

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