Frax Ether Liquid Staking contest - durianSausage's results

none-critical issue foundings

N01: EVENT IS MISSING INDEXED FIELDS

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 104, b' event MinterAdded(address minter_address);' src/ERC20/ERC20PermitPermissionedMint.sol, 105, b' event MinterRemoved(address minter_address);' src/ERC20/ERC20PermitPermissionedMint.sol, 106, b' event TimelockChanged(address timelock_address);' src/frxETHMinter.sol, 205, b' event EmergencyEtherRecovered(uint256 amount);' src/frxETHMinter.sol, 206, b' event EmergencyERC20Recovered(address tokenAddress, uint256 tokenAmount);' src/frxETHMinter.sol, 208, b' event DepositEtherPaused(bool new_status);' src/frxETHMinter.sol, 210, b' event SubmitPaused(bool new_status);' src/frxETHMinter.sol, 212, b' event WithholdRatioSet(uint256 newRatio);' src/OperatorRegistry.sol, 208, b' event TimelockChanged(address timelock_address);' src/OperatorRegistry.sol, 209, b' event WithdrawalCredentialSet(bytes _withdrawalCredential);' src/OperatorRegistry.sol, 210, b' event ValidatorAdded(bytes pubKey, bytes withdrawalCredential);' src/OperatorRegistry.sol, 211, b' event ValidatorArrayCleared();' src/OperatorRegistry.sol, 212, b' event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering);' src/OperatorRegistry.sol, 213, b' event ValidatorsPopped(uint256 times);' src/OperatorRegistry.sol, 214, b' event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);' src/OperatorRegistry.sol, 215, b' event KeysCleared();'

N02: NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

src/sfrxETH.sol, 2, b'pragma solidity ^0.8.0;' src/ERC20/ERC20PermitPermissionedMint.sol, 2, b'pragma solidity ^0.8.0;' src/frxETHMinter.sol, 2, b'pragma solidity ^0.8.0;' src/OperatorRegistry.sol, 2, b'pragma solidity ^0.8.0;' src/frxETH.sol, 2, b'pragma solidity ^0.8.0;' lib/ERC4626/src/xERC4626.sol, 4, b'pragma solidity ^0.8.0;'

L01: RETURN VALUES OF approve() NOT CHECKED

prof

approve() NOT CHECKED Not all IERC20 implementations revert() when there’s a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

problem

src/frxETHMinter.sol, 75, b' frxETHToken.approve(address(sfrxETHToken), msg.value);'

L02: address variable should check if it is zero MISSING CHECKS FOR ADDRESS(0X0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 34, b' timelock_address = _timelock_address;' src/ERC20/ERC20PermitPermissionedMint.sol, 96, b' timelock_address = _timelock_address;' src/frxETHMinter.sol, 60, b' depositContract = IDepositContract(depositContractAddress);' src/frxETHMinter.sol, 61, b' frxETHToken = frxETH(frxETHAddress);' src/frxETHMinter.sol, 62, b' sfrxETHToken = IsfrxETH(sfrxETHAddress);' src/OperatorRegistry.sol, 41, b' timelock_address = _timelock_address;' src/OperatorRegistry.sol, 204, b' timelock_address = _timelock_address;'

G01: custom error save more gas

problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained https://blog.soliditylang.org/2021/04/21/custom-errors/. Custom errors are defined using the error statement.

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 66, b' require(minter_address != address(0), "Zero address detected");' src/ERC20/ERC20PermitPermissionedMint.sol, 68, b' require(minters[minter_address] == false, "Address already exists");' src/ERC20/ERC20PermitPermissionedMint.sol, 77, b' require(minter_address != address(0), "Zero address detected");' src/ERC20/ERC20PermitPermissionedMint.sol, 78, b' require(minters[minter_address] == true, "Address nonexistant");' src/ERC20/ERC20PermitPermissionedMint.sol, 95, b' require(_timelock_address != address(0), "Zero address detected"); ' src/frxETHMinter.sol, 79, b" require(sfrxeth_recieved > 0, 'No sfrxETH was returned');" src/frxETHMinter.sol, 87, b' require(!submitPaused, "Submit is paused");' src/frxETHMinter.sol, 88, b' require(msg.value != 0, "Cannot submit 0");' src/frxETHMinter.sol, 122, b' require(!depositEtherPaused, "Depositing ETH is paused");' src/frxETHMinter.sol, 126, b' require(numDeposits > 0, "Not enough ETH in contract");' src/frxETHMinter.sol, 140, b' require(!activeValidators[pubKey], "Validator already has 32 ETH");' src/frxETHMinter.sol, 160, b' require (newRatio <= RATIO_PRECISION, "Ratio cannot surpass 100%");' src/frxETHMinter.sol, 167, b' require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");' src/frxETHMinter.sol, 171, b' require(success, "Invalid transfer");' src/frxETHMinter.sol, 193, b' require(success, "Invalid transfer");' src/frxETHMinter.sol, 200, b' require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");' src/OperatorRegistry.sol, 137, b' require(numVals != 0, "Validator stack is empty");' src/OperatorRegistry.sol, 182, b' require(numValidators() == 0, "Clear validator array first");' src/OperatorRegistry.sol, 203, b' require(_timelock_address != address(0), "Zero address detected");'

G02: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

src/frxETHMinter.sol, 79, b" require(sfrxeth_recieved > 0, 'No sfrxETH was returned');" src/frxETHMinter.sol, 126, b' require(numDeposits > 0, "Not enough ETH in contract");'

G03: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 84, b' for (uint i = 0; i < minters_array.length; i++){ '

G04: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

prof

src/frxETHMinter.sol, 97, b' currentWithheldETH += withheld_amt;' src/frxETHMinter.sol, 168, b' currentWithheldETH -= amount;' lib/ERC4626/src/xERC4626.sol, 67, b' storedTotalAssets -= amount;' lib/ERC4626/src/xERC4626.sol, 72, b' storedTotalAssets += amount;'

G05: ARRAY LENTH SHOULD USE MEMoRY VARIABLE LOAD IT

problem

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP (3 gas), and gets rid of the extra DUP needed to store the stack offset

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 84, b' for (uint i = 0; i < minters_array.length; i++){ '

G06: USING BOOLS FOR STORAGE INCURS OVERHEAD

problem

// 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.

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 20, b' mapping(address => bool) public minters; // Mapping is also used for faster verification' src/frxETHMinter.sol, 43, b' mapping(bytes => bool) public activeValidators; // Tracks validators (via their pubkeys) that already have 32 ETH in them'

G07: resign the default value to the variables.

problem

resign the default value to the variables will cost more gas.

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 84, b' for (uint i = 0; i < minters_array.length; i++){ ' src/frxETHMinter.sol, 94, b' uint256 withheld_amt = 0;' src/frxETHMinter.sol, 129, b' for (uint256 i = 0; i < numDeposits; ++i) {' src/OperatorRegistry.sol, 63, b' for (uint256 i = 0; i < arrayLength; ++i) {' src/OperatorRegistry.sol, 84, b' for (uint256 i = 0; i < times; ++i) {' src/OperatorRegistry.sol, 114, b' for (uint256 i = 0; i < original_validators.length; ++i) {'

G08: ++I/I++ SHOULD BE UNCHECKED{++I}/UNCHECKED{I++} WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR- AND WHILE-LOOPS

problem

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

prof

src/ERC20/ERC20PermitPermissionedMint.sol, 84, b' for (uint i = 0; i < minters_array.length; i++){ ' src/frxETHMinter.sol, 129, b' for (uint256 i = 0; i < numDeposits; ++i) {' src/OperatorRegistry.sol, 63, b' for (uint256 i = 0; i < arrayLength; ++i) {' src/OperatorRegistry.sol, 84, b' for (uint256 i = 0; i < times; ++i) {' src/OperatorRegistry.sol, 114, b' for (uint256 i = 0; i < original_validators.length; ++i) {'

G09: FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

problem

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

prof

src/frxETHMinter.sol, 196, b' function recoverEther(uint256 amount) external onlyByOwnGov ' src/frxETHMinter.sol, 203, b' function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov '

G10: USING PRIVATE RATHER THAN PUBLIC FOR CONSTANTS, SAVES GAS

problem:

We can save getter function of public constants.

prof:

src/frxETHMinter.sol, 38, b' uint256 public constant DEPOSIT_SIZE = 32 ether; // ETH 2.0 minimum deposit size' src/frxETHMinter.sol, 39, b' uint256 public constant RATIO_PRECISION = 1e6; // 1,000,000 ' src/frxETHMinter.sol, 45, b' IDepositContract public immutable depositContract; // ETH 2.0 deposit contract' src/frxETHMinter.sol, 46, b' frxETH public immutable frxETHToken;' src/frxETHMinter.sol, 47, b' IsfrxETH public immutable sfrxETHToken;' lib/ERC4626/src/xERC4626.sol, 24, b' uint32 public immutable rewardsCycleLength;'

G11: USAGE OF UINTS/INTS SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD

problem

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.

prof

src/sfrxETH.sol, 50, b' if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } ' src/sfrxETH.sol, 55, b' return convertToAssets(1e18);' src/sfrxETH.sol, 69, b' asset.permit(msg.sender, address(this), amount, deadline, v, r, s);' src/sfrxETH.sol, 85, b' asset.permit(msg.sender, address(this), amount, deadline, v, r, s);' src/ERC20/ERC20PermitPermissionedMint.sol, 54, b' super.burnFrom(b_address, b_amount);' src/ERC20/ERC20PermitPermissionedMint.sol, 60, b' super.mint(m_address, m_amount);' src/ERC20/ERC20PermitPermissionedMint.sol, 66, b' require(minter_address != address(0), "Zero address detected");' src/ERC20/ERC20PermitPermissionedMint.sol, 70, b' minters_array.push(minter_address);' src/ERC20/ERC20PermitPermissionedMint.sol, 77, b' require(minter_address != address(0), "Zero address detected");' src/ERC20/ERC20PermitPermissionedMint.sol, 84, b' for (uint i = 0; i < minters_array.length; i++){ ' src/ERC20/ERC20PermitPermissionedMint.sol, 95, b' require(timelock_address != address(0), "Zero address detected"); ' src/frxETHMinter.sol, 72, b' submit(address(this));' src/frxETHMinter.sol, 79, b" require(sfrxeth_recieved > 0, 'No sfrxETH was returned');" src/frxETHMinter.sol, 88, b' require(msg.value != 0, "Cannot submit 0");' src/frxETHMinter.sol, 94, b' uint256 withheld_amt = 0;' src/frxETHMinter.sol, 95, b' if (withholdRatio != 0) {' src/frxETHMinter.sol, 126, b' require(numDeposits > 0, "Not enough ETH in contract");' src/frxETHMinter.sol, 129, b' for (uint256 i = 0; i < numDeposits; ++i) {' src/OperatorRegistry.sol, 54, b' validators.push(validator);' src/OperatorRegistry.sol, 63, b' for (uint256 i = 0; i < arrayLength; ++i) {' src/OperatorRegistry.sol, 85, b' validators.pop();' src/OperatorRegistry.sol, 84, b' for (uint256 i = 0; i < times; ++i) {' src/OperatorRegistry.sol, 116, b' validators.push(original_validators[i]);' src/OperatorRegistry.sol, 114, b' for (uint256 i = 0; i < original_validators.length; ++i) {' src/OperatorRegistry.sol, 100, b' swapValidator(remove_idx, validators.length - 1);' src/OperatorRegistry.sol, 103, b' validators.pop();' src/OperatorRegistry.sol, 137, b' require(numVals != 0, "Validator stack is empty");' src/OperatorRegistry.sol, 141, b' validators.pop();' src/OperatorRegistry.sol, 176, b' return Validator(pubKey, signature, depositDataRoot);' src/OperatorRegistry.sol, 182, b' require(numValidators() == 0, "Clear validator array first");' src/OperatorRegistry.sol, 203, b' require(timelock_address != address(0), "Zero address detected");' lib/ERC4626/src/xERC4626.sol, 24, b' uint32 public immutable rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 27, b' uint32 public lastSync;' lib/ERC4626/src/xERC4626.sol, 30, b' uint32 public rewardsCycleEnd;' lib/ERC4626/src/xERC4626.sol, 33, b' uint192 public lastRewardAmount;' lib/ERC4626/src/xERC4626.sol, 38, b' rewardsCycleLength = rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 40, b' rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 48, b' uint192 lastRewardAmount = lastRewardAmount;' lib/ERC4626/src/xERC4626.sol, 48, b' uint192 lastRewardAmount = lastRewardAmount;' lib/ERC4626/src/xERC4626.sol, 49, b' uint32 rewardsCycleEnd = rewardsCycleEnd;' lib/ERC4626/src/xERC4626.sol, 49, b' uint32 rewardsCycleEnd = rewardsCycleEnd;' lib/ERC4626/src/xERC4626.sol, 50, b' uint32 lastSync = lastSync;' lib/ERC4626/src/xERC4626.sol, 50, b' uint32 lastSync_ = lastSync;' lib/ERC4626/src/xERC4626.sol, 52, b' if (block.timestamp >= rewardsCycleEnd_) {' lib/ERC4626/src/xERC4626.sol, 55, b' return storedTotalAssets_ + lastRewardAmount_;' lib/ERC4626/src/xERC4626.sol, 60, b' uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);' lib/ERC4626/src/xERC4626.sol, 79, b' uint192 lastRewardAmount_ = lastRewardAmount;' lib/ERC4626/src/xERC4626.sol, 79, b' uint192 lastRewardAmount_ = lastRewardAmount;' lib/ERC4626/src/xERC4626.sol, 80, b' uint32 timestamp = block.timestamp.safeCastTo32();' lib/ERC4626/src/xERC4626.sol, 80, b' uint32 timestamp = block.timestamp.safeCastTo32();' lib/ERC4626/src/xERC4626.sol, 82, b' if (timestamp < rewardsCycleEnd) revert SyncError();' lib/ERC4626/src/xERC4626.sol, 82, b' if (timestamp < rewardsCycleEnd) revert SyncError();' lib/ERC4626/src/xERC4626.sol, 85, b' uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;' lib/ERC4626/src/xERC4626.sol, 89, b' uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 89, b' uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 89, b' uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 89, b' uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;' lib/ERC4626/src/xERC4626.sol, 92, b' lastRewardAmount = nextRewards.safeCastTo192();' lib/ERC4626/src/xERC4626.sol, 93, b' lastSync = timestamp;' lib/ERC4626/src/xERC4626.sol, 94, b' rewardsCycleEnd = end;'