GithubLink:
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/ContestManager.sol#L16
Description:
The smart contract TestMyCut is designed to distribute rewards among participants in a contest. However, there is a critical flaw in the way the total rewards are calculated and validated. The totalRewards variable is set manually and is not dynamically calculated based on the sum of individual rewards (rewards array). This inconsistency can lead to either over-distribution or under-distribution of rewards, resulting in unclaimed rewards in the pool or participants not receiving their due rewards.
Impact:
Over-Distribution: If totalRewards is set higher than the sum of individual rewards, it can lead to an excess of funds in the prize pool that remains unclaimed. This not only affects the financial integrity of the contest but also can lead to potential misuse or loss of funds.
Under-Distribution: Conversely, if totalRewards is set lower than the sum required, participants may not receive their full rewards, leading to dissatisfaction and a lack of trust in the contest's fairness and reliability.
Proof of Concept:
Use Foundry Tools and Test File:
Tool Used: Foundry (forge 0.2.0)
Test File: TestMyCut.t.sol
In the provided test cases, in the function createContest(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards), uint256[] memory rewards are set to [3, 1], which should sum up to 4. However, totalRewards is manually set to 6 in one scenario and 3 in another.
Over-Funding Test:
The test testUnclaimedRewardDistribution with totalRewards = 6 was executed. The individual rewards were set as [3, 1], which should sum up to 4. However, totalRewards was manually set to 6.
pragma solidity ^0.8.20;
import {ContestManager} from "../src/ContestManager.sol";
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {Pot} from "../src/Pot.sol";
contract TestMyCut is Test {
address conMan;
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2];
uint256 public constant STARTING_USER_BALANCE = 1000 ether;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [3, 1];
address user = makeAddr("user");
uint256 totalRewards = 6;
function setUp() public {
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
console.log("User Address: ", user);
console.log("Contest Manager Address 1: ", address(conMan));
vm.stopPrank();
}
modifier mintAndApproveTokens() {
console.log("Minting tokens to: ", user);
vm.startPrank(user);
ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
ERC20Mock(weth).approve(conMan, STARTING_USER_BALANCE);
console.log("Approved tokens to: ", address(conMan));
vm.stopPrank();
_;
}
function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
uint256 poolBalanceAfter = ERC20Mock(weth).balanceOf(contest);
uint256 claimantBalanceafter = ERC20Mock(weth).balanceOf(player1);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(user);
assert(poolBalanceAfter ==0);
}
}
├─ [641] ERC20Mock::balanceOf(Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]) [staticcall]
│ └─ ← [Return] 2
├─ [641] ERC20Mock::balanceOf(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84]) [staticcall]
│ └─ ← [Return] 4
├─ [641] ERC20Mock::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← [Return] 999999999999999999994 [9.999e20]
└─ ← [Revert] panic: assertion failed (0x01)
The transaction logs show that Pot::claimCut() was called, transferring 3 tokens to player1. After the contest was closed using ContestManager::closeContest(), the remaining balance in the Pot contract was 2 tokens, indicating that there were unclaimed funds.
The assertion failed because the expected balance after closing the contest was 0, but it was 2, showing that the over-allocation of totalRewards led to unclaimed funds.
Under-Funding Test Execution:
Conducted a test with totalRewards set to 3, which is less than the sum of the rewards array.
After the execution of Pot::claimCut() for player1, an attempt was made to execute Pot::claimCut() for player2.
pragma solidity ^0.8.20;
import {ContestManager} from "../src/ContestManager.sol";
import {Test, console} from "lib/forge-std/src/Test.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {Pot} from "../src/Pot.sol";
contract TestMyCut is Test {
address conMan;
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address[] players = [player1, player2];
uint256 public constant STARTING_USER_BALANCE = 1000 ether;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [3, 1];
address user = makeAddr("user");
uint256 totalRewards = 3;
function setUp() public {
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 1000e8);
console.log("User Address: ", user);
console.log("Contest Manager Address 1: ", address(conMan));
vm.stopPrank();
}
modifier mintAndApproveTokens() {
console.log("Minting tokens to: ", user);
vm.startPrank(user);
ERC20Mock(weth).mint(user, STARTING_USER_BALANCE);
ERC20Mock(weth).approve(conMan, STARTING_USER_BALANCE);
console.log("Approved tokens to: ", address(conMan));
vm.stopPrank();
_;
}
function testUnclaimedRewardDistribution() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
vm.startPrank(player1);
Pot(contest).claimCut();
vm.stopPrank();
vm.startPrank(player2);
Pot(contest).claimCut();
vm.stopPrank();
uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
uint256 poolBalanceAfter = ERC20Mock(weth).balanceOf(contest);
uint256 claimantBalanceafter = ERC20Mock(weth).balanceOf(player1);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(user);
assert(poolBalanceAfter ==0);
}
}
├─ [0] VM::startPrank(player2: [0xEb0A3b7B96C1883858292F0039161abD287E3324])
│ └─ ← [Return]
├─ [804] Pot::claimCut()
│ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
└─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
This scenario highlights a critical issue in the smart contract's reward management system, where the total rewards allocated are either over-claimed or miscalculated, leading to a failure when trying to distribute rewards to subsequent players. The error indicates a need for better fund management and validation checks within the contract to prevent such occurrences and ensure that each participant receives their rightful share.
Recommended Mitigation:
1.Dynamic Calculation of Total Rewards: Modify the contract to automatically calculate totalRewards by summing up the values in the rewards array. This will ensure that the total rewards are always consistent with the intended distribution.
- 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
- Pot pot = new Pot(players, rewards, token, totalRewards);
+ // Calculate the sum of rewards
+ uint256 totalRewards = 0;
+ for (uint256 i = 0; i < rewards.length; i++) {
+ totalRewards += rewards[i];
+ }
+
+ // Create a new Pot contract with the dynamically calculated totalRewards
+ Pot pot = new Pot(players, rewards, token, totalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = totalRewards;
return address(pot);
}
In this diff format code modification:
The uint256 totalRewards parameter has been removed from the function signature.
Code to calculate totalRewards has been added within the function body, dynamically calculating the total reward amount by iterating over the rewards array.
The creation code for the Pot contract has been updated to use the dynamically calculated totalRewards value.
2.Validation Checks: Implement validation checks before distributing rewards to ensure that the totalRewards matches the calculated sum of individual rewards. This will prevent discrepancies and potential financial errors.
Here's how you can implement the suggested validation checks in the createContest function to ensure that the totalRewards matches the sum of the rewards array. This will help prevent discrepancies and potential financial errors:
function createContest(address[] memory players, uint256[] memory rewards, IERC20 token)
public
onlyOwner
returns (address)
{
+ // Calculate the sum of rewards
+ uint256 calculatedTotalRewards = 0;
+ for (uint256 i = 0; i < rewards.length; i++) {
+ calculatedTotalRewards += rewards[i];
+ }
+ // Validation check to ensure the provided totalRewards matches the calculated sum
+ require(calculatedTotalRewards == totalRewards, "Total rewards mismatch");
// Create a new Pot contract with the validated totalRewards
Pot pot = new Pot(players, rewards, token, calculatedTotalRewards);
contests.push(address(pot));
contestToTotalRewards[address(pot)] = calculatedTotalRewards;
return address(pot);
}
In this updated code:
A new variable calculatedTotalRewards is introduced to hold the sum of the rewards array.
A require statement is added to perform a validation check that ensures the calculatedTotalRewards matches the totalRewards provided during the contract creation. If there is a mismatch, the transaction will revert with an appropriate error message.
The Pot contract is created with the calculatedTotalRewards, ensuring that only the correct total rewards are used.
This approach ensures that the total rewards used in the contract are accurate and validated against the sum of individual rewards, thus preventing any potential financial discrepancies.
