Frax Ether Liquid Staking contest - Deivitto's results

A liquid ETH staking derivative designed to uniquely leverage the Frax Finance ecosystem.

General Information

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

Frax Finance

Findings Distribution

Researcher Performance

Rank: 56/133

Findings: 2

Award: $43.64

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Frax Finance

Findings Information

🌟 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

Awards

30.653 USDC - $30.65

Labels

bug

QA (Quality Assurance)

old-submission-method

External Links

Link to GitHub issue

QA

Low

Not checking 0 value would revert on `xERC4626` constructor

Impact

Revert on constructor that can be not expected neither checked

Proof of Concept

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L38

Recommended Mitigation Steps

Add a check for != 0 to avoid reverting when calling xERC4626 constructor

Order of operations can affect time

Impact

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.

Details

In general, this is a problem due to precision.

Precision of time can be affected end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L78-L97

Precision of time can be affected rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L40

Tools

Slither + manual analysis

Proof of Concept

Lack of precision due to the order of operations

Recommended Mitigation Steps

Reorder the operations. For more info: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply

Event emmited by ERC20 Tokens with fee on transfer won't display actual value

Vulnerability details

There are ERC20 tokens that charge fee for every transfer().

frxETHMinter.sol#recoverERC20() assumes that the received amount is the same as the transfer amount, then emit that value. However, the actual transferred amount can be lower for those tokens.

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L200

    function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov {
        require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

        emit EmergencyERC20Recovered(tokenAddress, tokenAmount);
    }

Recommendation

Consider comparing before and after balance to get the actual transferred amount.

Prevent div by 0

Impact

On several locations in the code precautions are not being taken to not divide by 0, this would revert the code if happens.

Proof of Concept

Navigate to the following contracts,

If 0 this would revert creating an instance of the contract as this happens on the constructor. https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L40 rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength;

Safety checks that rewardsCycleEnd != lastSync https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L60 uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);

INFORMATIONAL: Not checked but can't reach this state as it would revert on constructor https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L89 uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

Recommended Mitigation Steps

Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.

Variable shadows another variable

Summary

Name shadowing where two or more variables/functions share the same name could be confusing to developers and/or reviewers

Github Permalinks

ERC20PermitPermissionedMint._name shadows ERC20._name https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L27
ERC20PermitPermissionedMint._symbol shadows ERC20._symbol https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L28

Mitigation

Replace for example _name variable to _permissionedMint_name or a similar substitution

Missing checks for address(0x0) when assigning values to `address` state or `immutable` variables

Summary

Zero address should be checked for state variables, immutable variables. A zero address can lead into problems.

Github Permalinks

state https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L41

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L34

immutable address not being checked https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L53-L57

Mitigation

Check zero address before assigning or using it

block.timestamp used as time proxy

Summary

Risk of using block.timestamp for time should be considered.

Details

block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.

References

SWC ID: 116

Github Permalinks

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L78-L97

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L48-L51

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L45-L62

Mitigation

Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.
Consider using an oracle

Return value not being checked

Details

Return values not being checked may lead into unexpected behaviors with functions. Not events/Error are being emitted if that fails, so functions would be called even of not being working as expect as for example submitAndDeposit doesn't check for frxETHToken.approve() value.

Github permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L75

Mitigation

Check values and revert/emit events if needed

Informational

Comparison with a a boolean

Summary

There are a number of instances where a boolean variable/function is checked.

Details

This check can be further simplified from variable == false to !variable.
This check can be further simplified from variable == true to variable.

Github Permalink

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L46 require(minters[msg.sender] == true, "Only minters");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L78 require(minters[minter_address] == true, "Address nonexistant");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L68 require(minters[minter_address] == false, "Address already exists");

Mitigation

Simplify boolean comparisons in order to improve readability and save gas

Use of magic numbers is confusing and risky

Summary

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

Details

Values are hardcoded and would be more readable and maintainable if declared as a constant

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L55 return convertToAssets(1e18);

Mitigation

Replace magic hardcoded numbers with declared constants.

Missing indexed event parameters

Summary

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

Details

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L104 event MinterAdded(address minter_address);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L105 event MinterRemoved(address minter_address);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L106 event TimelockChanged(address timelock_address);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L205 event EmergencyEtherRecovered(uint256 amount);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L206 event EmergencyERC20Recovered(address tokenAddress, uint256 tokenAmount);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L208 event DepositEtherPaused(bool new_status);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L210 event SubmitPaused(bool new_status);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L212 event WithholdRatioSet(uint256 newRatio);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L208 event TimelockChanged(address timelock_address);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L209 event WithdrawalCredentialSet(bytes _withdrawalCredential);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L210 event ValidatorAdded(bytes pubKey, bytes withdrawalCredential);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L212 event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L213 event ValidatorsPopped(uint256 times);

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L214 event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);

Suggested to add some parameter as a good practice https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L215 event KeysCleared();

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L211 event ValidatorArrayCleared();

Mitigation

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

Different versions of pragma

Summary

Some of the contracts include an unlocked pragma, e.g., pragma solidity >=0.8.0.

Locking the pragma helps ensure that contracts are not accidentally deployed using an old compiler version with unfixed bugs.

Github Permalinks

All 6 files

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L4 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETH.sol#L2 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L2 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L2 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L2 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L2

Mitigation

Lock pragmas to a specific Solidity version. Consider converting ^ 0.8.0 into 0.8.10]

Bad order of code

Summary

Clearness of the code is important for the readability and maintainability. As Solidity guidelines says about declaration order: 1.Type declarations 2.State variables 3.Events 4.Modifiers 5.Functions

github permalink

events https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L100-L106 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L205-L212
modifiers expected before constructor https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L24-L48
modifiers expected before constructor
events https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L40-L215
receive expected at the very end https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L114-L116

Mitigation

Follow solidity style guidelines https://docs.soliditylang.org/en/v0.8.15/style-guide.html

Missing Natspec

Summary

Missing Natspec and regular comments affect readability and maintainability of a codebase.

Details

Contracts has partial or full lack of comments

Github Permalinks

Natspec @param

Natspec @return value

mitigation

Add @param descriptors
Add @return descriptors
Add Natspec comments.
Add inline comments.
Add comments for what the contract does

Unused code

Summary

Code that is not used should be removed

Details

import of IERC20 not used

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L5

Mitigation

Remove the code that is not used.

Maximum line length exceeded

Summary

Long lines should be wrapped to conform with Solidity Style guidelines.

Details

Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETH.sol#L29

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L31

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L32

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L33

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L34

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L35

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L86

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L33

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L35

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L36

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L43

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L138

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L200

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L207

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L36

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L37

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L40

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L81

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L93

Mitigation

Reduce line length to less than 99 at least to improve maintainability and readability of the code

Findings Information

🌟 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

Awards

12.994 USDC - $12.99

Labels

bug

G (Gas Optimization)

old-submission-method

External Links

Link to GitHub issue

GAS

Public function visibility can be made external

Summary

Functions should have the strictest visibility possible. Public functions may lead to more gas usage by forcing the copy of their parameters to memory from calldata.

Details

If a function is never called from the contract it should be marked as external. This will save gas.

Github Permalinks

Mitigation

Consider changing visibility from public to external.

use of custom errors rather than revert() / require() error message

Summary

Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.

Details

Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L41 require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L46 require(minters[msg.sender] == true, "Only minters");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L66 require(minter_address != address(0), "Zero address detected");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L77 require(minter_address != address(0), "Zero address detected");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L78 require(minters[minter_address] == true, "Address nonexistant");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L95 require(_timelock_address != address(0), "Zero address detected");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L79 require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L87 require(!submitPaused, "Submit is paused");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L88 require(msg.value != 0, "Cannot submit 0");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L122 require(!depositEtherPaused, "Depositing ETH is paused");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L126 require(numDeposits > 0, "Not enough ETH in contract");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L140 require(!activeValidators[pubKey], "Validator already has 32 ETH");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L167 require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L171 require(success, "Invalid transfer");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L193 require(success, "Invalid transfer");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L200 require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20 Transfer failed");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L46 require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L137 require(numVals != 0, "Validator stack is empty");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L182 require(numValidators() == 0, "Clear validator array first");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L203 require(_timelock_address != address(0), "Zero address detected");

Mitigation

replace each error message in a require by a custom error

duplicated require() check should be refactored

Summary

duplicated require() / revert() checks should be refactored to a modifier or function to save gas

Details

Event appears twice and can be reduced

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L66 require(minter_address != address(0), "Zero address detected"); https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L77 require(minter_address != address(0), "Zero address detected"); https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L95 require(_timelock_address != address(0), "Zero address detected");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L171 require(success, "Invalid transfer");

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L193 require(success, "Invalid transfer");

Mitigation

refactor this checks to different functions to save gas

use != rather than >0 for unsigned integers in require() statements

Summary

When the optimizer is enabled, gas is wasted by doing a greater-than operation, rather than a not-equals operation inside require() statements. When Using != , the optimizer is able to avoid the EQ, ISZERO, and associated operations, by relying on the JUMPI that comes afterwards, which itself checks for zero.

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L79 require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L126 require(numDeposits > 0, "Not enough ETH in contract");

Mitigation

Use != 0 rather than > 0 for unsigned integers in require() statements.

Reduce the size of error messages (Long revert Strings)

Summary

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.

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L167

Mitigation

Consider shortening the revert strings to fit in 32 bytes

usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

Summary

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.

Details

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size than downcast where needed

Github Permalinks

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L22-L33

Mitigation

Consider using some data type that uses 32 bytes, for example uint256

Using bools for storage incurs overhead {

Summary

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.

Details

Here is one example of OpenZeppelin about this optimization https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L49 bool public submitPaused;

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L50 bool public depositEtherPaused;

Mitigation

Consider using uint256 with values 0 and 1 rather than bool

Using private rather than public for constants saves gas

Summary

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

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L38 uint256 public constant DEPOSIT_SIZE = 32 ether; // ETH 2.0 minimum deposit size

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L39 uint256 public constant RATIO_PRECISION = 1e6; // 1,000,000

Mitigation

Consider replacing public for private in constants for gas saving.

Explicit initialization

Summary

It is not needed to initialize variables to the default value. Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Details

If a variable is not set/initialized, it is assumed to have the default value ( 0 for uint, false for bool, address(0) for address…).

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L94 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L63 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L64

Mitigation

Don't initialize variables to default value

Index initialized in for loop

Summary

In for loops is not needed to initialize indexes to 0 as it is the default uint/int value. This saves gas.

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L84 for (uint i = 0; i < minters_array.length; i++){

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L129 for (uint256 i = 0; i < numDeposits; ++i) {

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L63 for (uint256 i = 0; i < arrayLength; ++i) {

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L84 for (uint256 i = 0; i < times; ++i) {

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L114 for (uint256 i = 0; i < original_validators.length; ++i) {

Mitigation

Don't initialize variables to default value

use of i++ in loop rather than ++i

Summary

++i costs less gas than i++, especially when it's used in for loops

Details

using ++i doesn't affect the flow of regular for loops and improves gas cost

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L84 for (uint i = 0; i < minters_array.length; i++){ https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L71

Mitigation

Substitute to ++i

increments can be unchecked in loops

Summary

Unchecked operations as the ++i on for loops are cheaper than checked one.

Details

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline..

The code would go from:

for (uint256 i; i < numIterations; i++) {
// ...
}

to

for (uint256 i; i < numIterations;) {
  // ...
  unchecked { ++i; }
}
The risk of overflow is inexistent for a uint256 here.

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L84 for (uint i = 0; i < minters_array.length; i++){

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L129 for (uint256 i = 0; i < numDeposits; ++i) {

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L63 for (uint256 i = 0; i < arrayLength; ++i) {

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L84 for (uint256 i = 0; i < times; ++i) {

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L114 for (uint256 i = 0; i < original_validators.length; ++i) {

Mitigation

Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header

<array>.length should no be looked up in every loop of a for-loop

Summary

In loops not assigning the length to a variable so memory accessed a lot (caching local variables)

Details

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)

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/ERC20/ERC20PermitPermissionedMint.sol#L84 for (uint i = 0; i < minters_array.length; i++){

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L114 for (uint256 i = 0; i < original_validators.length; ++i) {

Mitigation

Assign the length of the array.length to a local variable in loops for gas savings

<X> += <Y> costs more gas than <X> = <X> + <Y> for state variables

Summary

x+=y costs more gas than x=x+y for state variables Same happens with -=

Github Permalinks

https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L72 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L97 https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L67 https://github.com/corddry/ERC4626/blob/2b2baba0fc480326a89251716f52d2cfa8b09230/src/xERC4626.sol#L72

Mitigation

Don't use += for state variables as it cost more gas.

Use calldata instead of memory for function parameters

Summary

It is generally cheaper to load variables directly from calldata, rather than copying them to memory.

Details

Only use memory if the variable needs to be modified

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L171-L177 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L181-L186

Mitigation

Use calldata rather than memory in external functions where the parameter is not modified but only read

Unused named returns

Summary

Using both named returns and a return statement isn’t necessary. Removing one of those can improve code clarity

Details

Also as returns variable is ignored, it wastes extra gas

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L70 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L70 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/sfrxETH.sol#L86

Mitigation

Remove return or returns when both used

Retrieve internal variables directly

Summary

Save ~30 gas per call to each function

Details

Rather than calling from inside the code the function, it can be directly read as for example validators.length rather than numValidators() in order to save gas

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L136 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L182 https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/OperatorRegistry.sol#L197-L199

Mitigation

Retrieve internal variables directly rather than calling the retrieving function

Variables should be cached when used several times

Summary

Variables read more than once improves gas usage when cached into local variable

Details

In loops or state variables, this is even more gas saving

Github Permalinks

submitPaused https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L178-L180

depositEtherPaused https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L185-L187

Mitigation

Cache variables used more than one into a local variable.

Make immutable state variables that do not change but assigned in the constructor

Summary

State variables which value isn't changed by any function in the contract but constructor, can be declared as a immutable state variable to save some gas during deployment.

Github Permalinks

https://github.com/code-423n4/2022-09-frax/blob/dc6684f77b4e9bd965e8862be7f5fb71473a4c4c/src/frxETHMinter.sol#L42

Mitigation

Add immutable to state variables that do not change but which value is assigned in constructor

Frax Ether Liquid Staking contest - Deivitto's results

A liquid ETH staking derivative designed to uniquely leverage the Frax Finance ecosystem.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-79] QA Report - $30.65

Findings Information

Awards

Labels

External Links

QA

Low

Not checking 0 value would revert on xERC4626 constructor

Impact

Proof of Concept

Recommended Mitigation Steps

Order of operations can affect time

Impact

Details

Tools

Proof of Concept

Recommended Mitigation Steps

Event emmited by ERC20 Tokens with fee on transfer won't display actual value

Vulnerability details

Recommendation

Prevent div by 0

Impact

Proof of Concept

Recommended Mitigation Steps

Variable shadows another variable

Summary

Github Permalinks

Mitigation

Missing checks for address(0x0) when assigning values to address state or immutable variables

Summary

Github Permalinks

Mitigation

block.timestamp used as time proxy

Summary

Details

References

Github Permalinks

Mitigation

Return value not being checked

Details

Github permalinks

Mitigation

Informational

Comparison with a a boolean

Summary

Details

Github Permalink

Mitigation

Use of magic numbers is confusing and risky

Summary

Details

Github Permalinks

Mitigation

Missing indexed event parameters

Summary

Details

Github Permalinks

Mitigation

Different versions of pragma

Summary

Github Permalinks

Mitigation

Bad order of code

Summary

github permalink

Mitigation

Missing Natspec

Summary

Details

Github Permalinks

Natspec @param

Natspec @return value

More documentation needed

mitigation

Not checking 0 value would revert on `xERC4626` constructor

Missing checks for address(0x0) when assigning values to `address` state or `immutable` variables