Github Link
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L32
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L57
Vulnerability Details
If not cached, the solidity compiler will always read the length of the array during each iteration.
Impact
That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).
proof code
before catched：
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
after catched:
uint 256 Player_length = i_players.length
for (uint256 i = 0; i < Player_length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
use Foundry to text
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 {Strings} from "lib/openzeppelin-contracts/contracts/utils/Strings.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;
ERC20Mock weth;
address contest;
address[] totalContests;
uint256[] rewards = [1, 10];
address user = makeAddr("user");
uint256 totalRewards = 11;
function setUp() public {
for (uint256 i = 3; i < 10000; i++) {
string memory playerName = string(abi.encodePacked("player", Strings.toString(i)));
address playerAddr = makeAddr(playerName);
players.push(playerAddr);
rewards.push(uint256(i));
}
vm.startPrank(user);
conMan = address(new ContestManager());
weth = new ERC20Mock("WETH", "WETH", msg.sender, 11);
uint256 managerBalanceafter = ERC20Mock(weth).balanceOf(conMan);
uint256 manageruserafter = ERC20Mock(weth).balanceOf(user);
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 testCanCreatePot() public mintAndApproveTokens {
console.log("Contest Manager Owner: ", ContestManager(conMan).owner());
console.log("msg.sender: ", msg.sender);
vm.startPrank(user);
uint gasStart = gasleft();
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), totalRewards);
uint gasAfterPot = gasleft();
uint gasUsedCreateAndFund = gasStart - gasAfterPot;
console.log("Gas used for create and fund: %d", gasUsedCreateAndFund);
totalContests = ContestManager(conMan).getContests();
vm.stopPrank();
assertEq(totalContests.length, 1);
}
├─ [0] console::log("Gas used for create and fund: %d", 731507944 [7.315e8]) [staticcall]
After:
├─ [0] console::log("Gas used for create and fund: %d", 730508049 [7.305e8]) [staticcall]
To calculate the specific amount of gas saved after the function optimization, we can subtract the lower gas usage from the higher one.This calculation shows that the optimization saved approximately 999,895 gas units.
English Translation:
After the function was optimized by caching the array length, a significant amount of gas was saved when deploying the contract. Assuming 10,000 participants were involved in the reward claim process, the specific amount of gas saved can be calculated by subtracting the gas usage after the optimization from the initial gas usage:
[ \text{Gas Saved} = 731,507,944 - 730,508,049 ]
[ \text{Gas Saved} = 999,895 ]
This indicates that the optimization resulted in a savings of approximately 999,895 gas units.
Recommendations
Here's the original code with the necessary change to cache the length of the array for optimization:
-for (uint256 i = 0; i <i_players.length; i++) {
- playersToRewards[i_players[i]] = i_rewards[i];
-}
// ++++ Cache the array length to avoid repeated reads.
+uint256 playersLength = i_players.length; // Cache the length
+for (uint256 i = 0; i < playersLength; i++) {
+ playersToRewards[i_players[i]] = i_rewards[i];
+}
// ----- No need to read the length in each iteration.
By caching the length of the i_players array, you ensure that it's only read once, rather than on each iteration of the loop. This small change can lead to a significant reduction in gas costs, especially when dealing with large arrays.
