Platform: Code4rena
Start Date: 22/09/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 133
Period: 3 days
Judge: 0xean
Total Solo HM: 2
Id: 165
League: ETH
Rank: 51/133
Findings: 2
Award: $51.61
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: rotcivegaf
Also found by: 0x040, 0x1f8b, 0x4non, 0xNazgul, 0xSmartContract, 0xf15ers, 8olidity, Aymen0909, B2, Bahurum, Bnke0x0, Ch_301, CodingNameKiki, Deivitto, Diana, Funen, IllIllI, JC, JLevick, KIntern_NA, Lambda, OptimismSec, PaludoX0, RockingMiles, Rolezn, Sm4rty, Soosh, Tagir2003, Tointer, TomJ, Triangle, Trust, V_B, Waze, Yiko, __141345__, a12jmx, ajtra, asutorufos, ayeslick, aysha, bbuddha, bharg4v, bobirichman, brgltd, bytera, c3phas, cryptostellar5, cryptphi, csanuragjain, datapunk, delfin454000, durianSausage, exd0tpy, gogo, got_targ, jag, joestakey, karanctf, ladboy233, leosathya, lukris02, mics, millersplanet, natzuu, neko_nyaa, obront, oyc_109, parashar, peritoflores, rbserver, ret2basic, rokinot, ronnyx2017, rvierdiiev, sach1r0, seyni, sikorico, slowmoses, tnevler, yasir, yongskiws
32.7481 USDC - $32.75
File: /src/ERC20/ERC20PermitPermissionedMint.sol 34: timelock_address = _timelock_address;
File: /src/OperatorRegistry.sol 41: timelock_address = _timelock_address;
File: /src/frxETHMinter.sol 60: depositContract = IDepositContract(depositContractAddress); 61: frxETHToken = frxETH(frxETHAddress); 62: sfrxETHToken = IsfrxETH(sfrxETHAddress);
There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.
Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.
File: /src/sfrxETH.sol //@audit: 1e18 55: return convertToAssets(1e18);
Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
Consider adding a check to prevent accidentally burning tokens here:
File: /src/frxETHMinter.sol 166: function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov { 167: require(amount <= currentWithheldETH, "Not enough withheld ETH in contract"); 168: currentWithheldETH -= amount; 170: (bool success,) = payable(to).call{ value: amount }(""); 171: require(success, "Invalid transfer"); 173: emit WithheldETHMoved(to, amount); 174: }
Contracts are allowed to override their parents' functions and change the visibility from external to public. Functions marked by external use call data to read arguments, where public will first allocate in local memory and then load them.
File: /src/sfrxETH.sol 54: function pricePerShare() public view returns (uint256) {
File: /src/ERC20/ERC20PermitPermissionedMint.sol 53: function minter_burn_from(address b_address, uint256 b_amount) public onlyMinters { 65: function addMinter(address minter_address) public onlyByOwnGov { 76: function removeMinter(address minter_address) public onlyByOwnGov { 94: function setTimelock(address _timelock_address) public onlyByOwnGov {
File: /src/OperatorRegistry.sol 82: function popValidators(uint256 times) public onlyByOwnGov { 93: function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov {
Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.
File: /src/frxETH.sol 2:pragma solidity ^0.8.0;
File: /src/sfrxETH.sol 2:pragma solidity ^0.8.0;
File: /src/ERC20/ERC20PermitPermissionedMint.sol 2:pragma solidity ^0.8.0;
File: /src/frxETHMinter.sol 2:pragma solidity ^0.8.0;
File: /src/OperatorRegistry.sol 2:pragma solidity ^0.8.0;
https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L4
File: /src/xERC4626.sol 4:pragma solidity ^0.8.0;
Using both named returns and a return statement isn’t necessary in a function.To improve code quality, consider using only one of those.
File: /src/sfrxETH.sol //@audit: Named shares 59: function depositWithSignature( 60: uint256 assets, 61: address receiver, 62: uint256 deadline, 63: bool approveMax, 64: uint8 v, 65: bytes32 r, 66: bytes32 s 67: ) external nonReentrant returns (uint256 shares) {
File: /src/sfrxETH.sol //@audit: Named assets 75: function mintWithSignature( 76: uint256 shares, 77: address receiver, 78: uint256 deadline, 79: bool approveMax, 80: uint8 v, 81: bytes32 r, 82: bytes32 s 83: ) external nonReentrant returns (uint256 assets) {
File: /src/frxETHMinter.sol //@audit: Named shares, not used anywhere 70: function submitAndDeposit(address recipient) external payable returns (uint256 shares) {
Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables and more.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). see official docs
File: /src/sfrxETH.sol //@audit: Missing @param assets,@param receiver,@param deadline,@param approveMax,@param v,@param r,@param s //@audit: Missing @return shares 59: function depositWithSignature( 60: uint256 assets, 61: address receiver, 62: uint256 deadline, 63: bool approveMax, 64: uint8 v, 65: bytes32 r, 66: bytes32 s 67: ) external nonReentrant returns (uint256 shares) {
File: /src/sfrxETH.sol //@audit: Missing @param shares,@param receiver,@param deadline,@param approveMax,@param v,@param r,@param s //@audit: Missing @return 75: function mintWithSignature( 76: uint256 shares, 77: address receiver, 78: uint256 deadline, 79: bool approveMax, 80: uint8 v, 81: bytes32 r, 82: bytes32 s 83: ) external nonReentrant returns (uint256 assets) {
File: /src/ERC20/ERC20PermitPermissionedMint.sol //@audit: Missing @param b_address,@param b_amount 53: function minter_burn_from(address b_address, uint256 b_amount) public onlyMinters { //@audit: Missing @param m_address,@param m_amount 59: function minter_mint(address m_address, uint256 m_amount) public onlyMinters { //@audit: Missing @param minter_address 65: function addMinter(address minter_address) public onlyByOwnGov { //@audit: Missing @param minter_address 76: function removeMinter(address minter_address) public onlyByOwnGov { //@audit: Missing @param _timelock_address 94: function setTimelock(address _timelock_address) public onlyByOwnGov {
File: /src/frxETHMinter.sol //@audit: Missing @param recipient,@return shares 70: function submitAndDeposit(address recipient) external payable returns (uint256 shares) { //@audit: Missing @param recipient 109: function submitAndGive(address recipient) external payable { //@audit: Missing @param newRatio 159: function setWithholdRatio(uint256 newRatio) external onlyByOwnGov { //@audit: Missing @param to,@param amount 166: function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov { //@audit: Missing @param amount 191: function recoverEther(uint256 amount) external onlyByOwnGov { //@audit: Missing @param tokenAddress, @param tokenAmount 199: function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov {
File: /src/OperatorRegistry.sol //@audit: Missing @param validator 53: function addValidator(Validator calldata validator) public onlyByOwnGov { //@audit: Missing @param validatorArray 61: function addValidators(Validator[] calldata validatorArray) external onlyByOwnGov { //@audit: Missing @param from_idx, @param to_idx 69: function swapValidator(uint256 from_idx, uint256 to_idx) public onlyByOwnGov { //@audit: Missing @param times 82: function popValidators(uint256 times) public onlyByOwnGov { //@audit: Missing @param remove_idx,@param dont_care_about_ordering 93: function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov { //@audit: Missing @param i 151: function getValidator(uint i) //@audit: Missing @param pubkey, @param signature,@param depositDataRoot 171: function getValidatorStruct( 172: bytes memory pubKey, 173: bytes memory signature, 174: bytes32 depositDataRoot 175: ) external pure returns (Validator memory) { //@audit: Missing @param _new_withdrawal_pubkey 181: function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov { //@audit: Missing @param _timelock_address 202: function setTimelock(address _timelock_address) external onlyByOwnGov {
File: /src/OperatorRegistry.sol //@audit: removed ---> remove 27:/// @notice A permissioned owner can add and removed them at will
The best-practice layout for a contract should follow the following order: state variables, events, modifiers, constructor and functions. Function ordering helps readers identify which functions they can call and find constructor and fallback functions easier. Functions should be grouped according to their visibility and ordered as: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private. Some constructs deviate from this recommended best-practice: Modifiers are in the middle of contracts. External/public functions are mixed with internal/private ones. Few events are declared at the bottom
External/public functions are mixed with internal/private ones and Events are declared at the bottom https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol
External/public functions are mixed with internal/private ones https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol
Events are declared at the bottom https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L100 External/public functions are mixed with internal/private ones https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol
External/public functions are mixed with internal/private ones https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol
See Official docs
File: /src/OperatorRegistry.sol 151: function getValidator(uint i) 152: view 153: external 154: returns ( 155: bytes memory pubKey, 156: bytes memory withdrawalCredentials, 157: bytes memory signature, 158: bytes32 depositDataRoot 159: ) 160: {
Consider adopting recommended best-practice for code structure and layout.
Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
File: /src/OperatorRegistry.sol 208: event TimelockChanged(address timelock_address); 209: event WithdrawalCredentialSet(bytes _withdrawalCredential); 210: event ValidatorAdded(bytes pubKey, bytes withdrawalCredential); 212: event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering); 213: event ValidatorsPopped(uint256 times); 214: event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);
🌟 Selected for report: pfapostol
Also found by: 0x040, 0x1f8b, 0x4non, 0x5rings, 0xA5DF, 0xNazgul, 0xSmartContract, 0xmatt, 0xsam, Amithuddar, Aymen0909, B2, Ben, Bnke0x0, Chom, CodingNameKiki, Deivitto, Diana, Fitraldys, Funen, IllIllI, JAGADESH, JC, Metatron, Ocean_Sky, PaludoX0, Pheonix, RaymondFam, ReyAdmirado, RockingMiles, Rohan16, Rolezn, Satyam_Sharma, Sm4rty, SnowMan, SooYa, Tagir2003, TomJ, Tomio, Triangle, V_B, Waze, __141345__, ajtra, albincsergo, asutorufos, aysha, beardofginger, bobirichman, brgltd, bulej93, bytera, c3phas, ch0bu, cryptostellar5, cryptphi, d3e4, delfin454000, dharma09, drdr, durianSausage, emrekocak, erictee, fatherOfBlocks, gogo, got_targ, imare, jag, karanctf, ladboy233, leosathya, lukris02, medikko, mics, millersplanet, natzuu, neko_nyaa, oyc_109, peanuts, prasantgupta52, rbserver, ret2basic, rokinot, ronnyx2017, rotcivegaf, sach1r0, samruna, seyni, slowmoses, tnevler, wagmi, zishansami
18.8604 USDC - $18.86
NB: Some functions have been truncated where necessary to just show affected parts of the code
Throughout the report some places might be denoted with audit tags to show the actual place affected.
The code can be optimized by minimizing the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
NB: Some functions have been truncated where neccessary to just show affected parts of the code
File: /src/frxETHMinter.sol 85: function _submit(address recipient) internal nonReentrant { 95: if (withholdRatio != 0) { //@audit: withholdRatio SLOAD 1 96: withheld_amt = (msg.value * withholdRatio) / RATIO_PRECISION; //@audit: withholdRatio SLOAD 2 97: currentWithheldETH += withheld_amt; 98: } 101: }
diff --git a/src/frxETHMinter.sol b/src/frxETHMinter.sol index 4565883..c49f0a6 100644 --- a/src/frxETHMinter.sol +++ b/src/frxETHMinter.sol @@ -92,8 +92,9 @@ contract frxETHMinter is OperatorRegistry, ReentrancyGuard { // Track the amount of ETH that we are keeping uint256 withheld_amt = 0; - if (withholdRatio != 0) { - withheld_amt = (msg.value * withholdRatio) / RATIO_PRECISION; + uint256 tempWithholdRatio = withholdRatio; + if (tempWithholdRatio != 0) { + withheld_amt = (msg.value * tempWithholdRatio) / RATIO_PRECISION; currentWithheldETH += withheld_amt; }
File: /src/frxETHMinter.sol 166: function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov { 167: require(amount <= currentWithheldETH, "Not enough withheld ETH in contract"); 168: currentWithheldETH -= amount; 174: }
diff --git a/src/frxETHMinter.sol b/src/frxETHMinter.sol index 4565883..dc3f2a7 100644 --- a/src/frxETHMinter.sol +++ b/src/frxETHMinter.sol @@ -164,8 +164,9 @@ contract frxETHMinter is OperatorRegistry, ReentrancyGuard { /// @notice Give the withheld ETH to the "to" address function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov { - require(amount <= currentWithheldETH, "Not enough withheld ETH in contract"); - currentWithheldETH -= amount; + uint256 tempWithheldEth = currentWithheldETH; + require(amount <= tempWithheldEth, "Not enough withheld ETH in contract"); + currentWithheldETH = tempWithheldEth - amount; (bool success,) = payable(to).call{ value: amount }(""); require(success, "Invalid transfer");
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
Affected code:
File: /src/OperatorRegistry.sol 126: function getNextValidator() 127: internal 128: returns ( 129: bytes memory pubKey, 130: bytes memory withdrawalCredentials, 131: bytes memory signature, 132: bytes32 depositDataRoot 133: )
The above function is only called once on a child contract at Line 136
If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.
File: /src/OperatorRegistry.sol //@audit: Use calldata on pubKey and signature 171: function getValidatorStruct( 172: bytes memory pubKey, 173: bytes memory signature, 174: bytes32 depositDataRoot 175: ) external pure returns (Validator memory) { 176: return Validator(pubKey, signature, depositDataRoot); 177: }
File: /src/OperatorRegistry.sol //@audit: Use calldata on _new_withdrawal_pubkey 181: function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov { 182: require(numValidators() == 0, "Clear validator array first"); 183: curr_withdrawal_pubkey = _new_withdrawal_pubkey; 185: emit WithdrawalCredentialSet(_new_withdrawal_pubkey); 186: }
File: /src/xERC4626.sol 78: function syncRewards() public virtual { 79: uint192 lastRewardAmount_ = lastRewardAmount; 80: uint32 timestamp = block.timestamp.safeCastTo32(); 82 if (timestamp < rewardsCycleEnd) revert SyncError(); ... 97: }
In the above function, we could move the uint192 lastRewardAmount_ = lastRewardAmount; to be run after the if statement. This way if the check on the if statement doesn't succeed then we don't need to incur the gas for the assignment of lastRewardAmount_ which involves reading from storage
diff --git a/src/xERC4626.sol b/src/xERC4626.sol index a8a4726..83c6738 100644 --- a/src/xERC4626.sol +++ b/src/xERC4626.sol @@ -76,10 +76,10 @@ abstract contract xERC4626 is IxERC4626, ERC4626 { /// @notice Distributes rewards to xERC4626 holders. /// All surplus `asset` balance of the contract over the internal balance becomes queued for the next cycle. function syncRewards() public virtual { - uint192 lastRewardAmount_ = lastRewardAmount; uint32 timestamp = block.timestamp.safeCastTo32(); if (timestamp < rewardsCycleEnd) revert SyncError(); + uint192 lastRewardAmount_ = lastRewardAmount;
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
File: /src/OperatorRegistry.sol 71: Validator memory fromVal = validators[from_idx]; 72: Validator memory toVal = validators[to_idx]; 140: Validator memory popped = validators[numVals - 1]; 161: Validator memory v = validators[i];
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
File: /src/frxETHMinter.sol 78: uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);
File: /src/frxETHMinter.sol 200: require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");
File: /src/frxETHMinter.sol 97: currentWithheldETH += withheld_amt;
Modify the above as
currentWithheldETH = currentWithheldETH + withheld_amt;
File: /src/frxETHMinter.sol 168: currentWithheldETH -= amount;
File: /src/xERC4626.sol 67: storedTotalAssets -= amount;
File: /src/xERC4626.sol 72: storedTotalAssets += amount;
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: /src/sfrxETH.sol //@audit: uint8 v, 59: function depositWithSignature( 60: uint256 assets, 61: address receiver, 62: uint256 deadline, 63: bool approveMax, 64: uint8 v, 65: bytes32 r, 66: bytes32 s 67: ) external nonReentrant returns (uint256 shares) {
File: /src/sfrxETH.sol //@audit: Missing @param shares,@param receiver,@param deadline,@param approveMax,@param v,@param r,@param s //@audit: Missing @return 75: function mintWithSignature( 76: uint256 shares, 77: address receiver, 78: uint256 deadline, 79: bool approveMax, 80: uint8 v, 81: bytes32 r, 82: bytes32 s 83: ) external nonReentrant returns (uint256 assets) {
File: /src/xERC4626.sol //@audit: uint32 _rewardsCycleLength 37: constructor(uint32 _rewardsCycleLength) { 48: uint192 lastRewardAmount_ = lastRewardAmount; 49: uint32 rewardsCycleEnd_ = rewardsCycleEnd; 50: uint32 lastSync_ = lastSync; 79: uint192 lastRewardAmount_ = lastRewardAmount; 80: uint32 timestamp = block.timestamp.safeCastTo32(); 89: uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied Declaring uint256 i = 0; means doing an MSTORE of the value 0 Instead you could just declare uint256 i to declare the variable without assigning it’s default value, saving 3 gas per declaration
File: /src/frxETHMinter.sol 94: uint256 withheld_amt = 0; //@audit: uint256 i = 0 129: for (uint256 i = 0; i < numDeposits; ++i) {
File: /src/ERC20/ERC20PermitPermissionedMint.sol //@audit: uint i = 0 84: for (uint i = 0; i < minters_array.length; i++){
File: /src/OperatorRegistry.sol 63: for (uint256 i = 0; i < arrayLength; ++i) { 84: for (uint256 i = 0; i < times; ++i) { 114: for (uint256 i = 0; i < original_validators.length; ++i) {
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource
File: /src/frxETHMinter.sol 168: currentWithheldETH -= amount;
The above operation cannot underflow due to the check on Line 167 that ensures that currentWithheldETH is greater than amount before performing the subtraction
File: /src/OperatorRegistry.sol 140: Validator memory popped = validators[numVals - 1];
The operation numVals - 1 cannot underflow due to the check on Line 137 that ensures that numVals is not 0 before performing the subtraction operation
The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.
e.g Let's work with a sample loop below.
for(uint256 i; i < 10; i++){ //doSomething }
can be written as shown below.
for(uint256 i; i < 10;) { // loop logic unchecked { i++; } }
We can also write it as an inlined function like below.
function inc(i) internal pure returns (uint256) { unchecked { return i + 1; } } for(uint256 i; i < 10; i = inc(i)) { // doSomething }
File: /src/ERC20/ERC20PermitPermissionedMint.sol 84: for (uint i = 0; i < minters_array.length; i++){
File: /src/frxETHMinter.sol 129: for (uint256 i = 0; i < numDeposits; ++i) {
File: /src/OperatorRegistry.sol 63: for (uint256 i = 0; i < arrayLength; ++i) {
File: /src/OperatorRegistry.sol 84: for (uint256 i = 0; i < times; ++i) {
File: /src/OperatorRegistry.sol 114: for (uint256 i = 0; i < original_validators.length; ++i) {
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
The solidity compiler will always read the length of the array during each iteration. That is,
1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)
This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas
Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:
File: /src/ERC20/ERC20PermitPermissionedMint.sol 84: for (uint i = 0; i < minters_array.length; i++){
File: /src/OperatorRegistry.sol 114: for (uint256 i = 0; i < original_validators.length; ++i) {
++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.
i++ increments i and returns the initial value of i. Which means:
uint i = 1; i++; // == 1 but i == 2
But ++i returns the actual incremented value:
uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable
In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2
Instances include:
File: /src/ERC20/ERC20PermitPermissionedMint.sol 84: for (uint i = 0; i < minters_array.length; i++){
Up until Solidity 0.8.13: != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)
For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.
Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706
I suggest changing > 0 with != 0 here:
File: /src/frxETHMinter.sol 79: require(sfrxeth_recieved > 0, 'No sfrxETH was returned'); 126: require(numDeposits > 0, "Not enough ETH in contract");
Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:
File: /src/ERC20/ERC20PermitPermissionedMint.sol 46: require(minters[msg.sender] == true, "Only minters");
File: /src/ERC20/ERC20PermitPermissionedMint.sol 68: require(minters[minter_address] == false, "Address already exists");
File: /src/ERC20/ERC20PermitPermissionedMint.sol 78: require(minters[minter_address] == true, "Address nonexistant");
// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past
Instances affected include https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L20
File: /src/ERC20/ERC20PermitPermissionedMint.sol 20: mapping(address => bool) public minters; // Mapping is also used for faster verification
File: /src/frxETHMinter.sol 43: mapping(bytes => bool) public activeValidators; // Tracks validators (via their pubkeys) that already have 32 ETH in them
File: /src/frxETHMinter.sol 49: bool public submitPaused; 50: bool public depositEtherPaused;
If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.
File: /src/frxETHMinter.sol 38: uint256 public constant DEPOSIT_SIZE = 32 ether; // ETH 2.0 minimum deposit size 39: uint256 public constant RATIO_PRECISION = 1e6; // 1,000,000
Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.Each extra chunk of byetes past the original 32 incurs an MSTORE which costs 3 gas
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
File: /src/frxETHMinter.sol 167: require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
The entire codebase seems to be using the following
pragma solidity ^0.8;
I've highlighted the places where we can utilize the custom errors following the above.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source
File: /src/ERC20/ERC20PermitPermissionedMint.sol 41: require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock"); 46: require(minters[msg.sender] == true, "Only minters"); 66: require(minter_address != address(0), "Zero address detected"); 68: require(minters[minter_address] == false, "Address already exists"); 77: require(minter_address != address(0), "Zero address detected"); 78: require(minters[minter_address] == true, "Address nonexistant"); 95: require(_timelock_address != address(0), "Zero address detected");
File: /src/OperatorRegistry.sol 46: require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock"); 137: require(numVals != 0, "Validator stack is empty"); 182: require(numValidators() == 0, "Clear validator array first"); 203: require(_timelock_address != address(0), "Zero address detected");