Fraxlend (Frax Finance) contest - Deivitto's results

Fraxlend: A permissionless lending platform and the final piece of the Frax Finance Defi Trinity.

General Information

Platform: Code4rena

Start Date: 12/08/2022

Pot Size: $50,000 USDC

Total HM: 15

Participants: 120

Period: 5 days

Judge: Justin Goro

Total Solo HM: 6

Id: 153

League: ETH

Frax Finance

Findings Distribution

Researcher Performance

Rank: 25/120

Findings: 2

Award: $183.97

🌟 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: 0x1f8b

Also found by: 0x52, 0xA5DF, 0xDjango, 0xNazgul, 0xNineDec, 0xSmartContract, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, EthLedger, Funen, IllIllI, JC, Junnon, Lambda, LeoS, MiloTruck, Noah3o6, PaludoX0, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, SaharAP, Sm4rty, SooYa, The_GUILD, TomJ, Waze, Yiko, _Adam, __141345__, a12jmx, ak1, asutorufos, auditor0517, ayeslick, ballx, beelzebufo, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, cccz, cryptonue, cryptphi, d3e4, delfin454000, dipp, djxploit, durianSausage, dy, erictee, fatherOfBlocks, gogo, gzeon, hyh, ignacio, kyteg, ladboy233, medikko, mics, minhquanym, oyc_109, pfapostol, rbserver, reassor, ret2basic, robee, sach1r0, simon135, sryysryy, tabish, yac, yash90, zzzitron

Awards

99.4236 USDC - $99.42

Labels

bug

QA (Quality Assurance)

old-submission-method

External Links

Link to GitHub issue

QA

Low

approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()

Summary

approve is subject to a known front-running attack.

Github permalink

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1103 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1184

Mitigation

Consider using safeApprove instead. Keep in mind though that it would be actually better to replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance(). See this discussion: SafeERC20.safeApprove() Has unnecessary and unsecure added behavior: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219

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

immutable variables https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L107

state variables https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L104-L106

params that affect both cases and not checked https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L152-L153

Mitigation

Check zero address before assigning or using it

Missing checks for address(0x0) in withdraw functions

Summary

Zero address should be checked for some function parameters. For example in functions like mints, withdrawals...

A zero address can lead into serious problems as locking eth or correct functioning.

Details

Missing of checks can lead into loss of funds:

There are no checks of 0 address in withdraw
There are no checks of 0 address in withdrawFees

Github Permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L676-L680 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L234 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L623-L629

Mitigation

Check zero address before assigning or using it

Strict equalities are dangerous in manipulable values

Summary:

Using == rather than >= or <= is not ideal in some cases where values can be manipulated

Details:

Strict equalities using block.timestamp or amounts is not ideal as they can be manipulated so the equality is always avoided.

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

Github Permalinks:

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L420 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L518

Mitigation:

Avoid the use of strict equalities in values that can be manipulated

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.

This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.

References

SWC ID: 116

Github Permalinks:

Mitigation:

Consider using an oracle for precision 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.

Front run initializer

Summary

The initialize function that initializes important contract state can be called by anyone.

Details

FraxlendPair, what is FraxlendPairCore that is where initialize() is declared, is not IFraxlendPair, so creating in FraxlendPairDeployer an IFraxlendPair doesn't mean to create a FraxlendPair that means a) or an interface is missing or b) is supposed to be declared elsewhere and the initialize can fall vs a front run.

The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.

In the best case for the victim, they notice it and have to redeploy their contract costing gas.

Github Permalinks

initialize() cast https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L258-L259
initialize() declaration https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L245
probably missing interface https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L41

Mitigation

Consider what is the problem: If it is being missed an interface, add it. Else Use the constructor to initialize non-proxied contracts. For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.

Naming convention of state variable non constant

Summary:

Only constants are suggested to use style CONSTANTS_WITH_UNDERSCORES, other variables are suggested to use camelCase.

If this variables in CONSTANTS_WITH_UNDERSCORES are supposed to be constant, add the constant keyword.

Also, guidelines doesn't say to use uppercase on immutables. Also, in case you prefer to use them no matter what, be consistent with the camelCase immutable variables so they are also in caps.

Github Permalinks:

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L49-L51

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L81-L89

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L57-L62

Mitigation

Rename to camelCase

abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccack256()

Summary

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456) , but abi.encode(0x123,0x456) => 0x0...1230...456 ). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

Details

abi.encodePacked will only use the only use the minimal required memory to encode the data. E.g. an address will only use 20 bytes and for dynamic arrays only the elements will be stored without length. For more info see the Solidity docs for packed mode.

For the input of the keccak method it is important that you can ensure that the resulting bytes of the encoding are unique. So if you always encode the same types and arrays always have the same length then there is no problem. But if you switch the parameters that you encode or encode multiple dynamic arrays you might have conflicts.

https://ethereum.stackexchange.com/questions/119583/when-to-use-abi-encode-abi-encodepacked-or-abi-encodewithsignature-in-solidity

Github permalinks

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L329 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L372

mitigation

Change abi.encodePacked to abi.encode when data collision may happen

Informational

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

In several locations in the code numbers like 18, 90, 1e36, 13000, 32, 1e18, 64 are used.

Github Permalinks:

18 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L85 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L57

90 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L194

1e36 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L522

13000 -> 3 times https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L171-L174

32 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L226 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L21 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L23 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L27

1e18 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/VariableInterestRate.sol#L69 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/VariableInterestRate.sol#L76

64 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L19

Mitigation:

Replace magic hardcoded numbers with declared constants. Define constants for the numbers used throughout the code. Comment what this numbers are intended for

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-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L200 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L211 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L228 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L268 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L376-L382 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L389 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L504

I would add more indexed values because of the big amount of params and some of them being important https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1143-L1149 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1045-L1052 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L897-L904

Mitigation:

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

Use of string.concat() rather than abi.encodePacked(<str>, <str>)

Summary

string.concat() can be used to join strings

Details

string.concat() can be used instead of abi.encodePacked(<str>,<str>)

Github Permalinks

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L316-L325

Mitigation

Replace to string.concat() where concatenation of strings is the goal.

Implementation of the comment is confusing

Summary:

Different comments says something that is confuse

Details

uint16 internal constant DEFAULT_PROTOCOL_FEE = 0; // 1e5 precision I don't understand why it says that the precision of 0 is 1e5

    uint256 private constant MIN_INT = 0; // 0.00% annual rate
    uint256 private constant MAX_INT = 146248508681; // 10,000% annual rate @audit I don't understand why 146248508681 is a 10,000%, is this the result of a date * rate operation?

Github Permalinks:

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairConstants.sol#L47 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L34

Mitigation:

Consider explaining more, correcting the code or the comments as needed.

Missing Natspec

Summary:

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

Details:

Contracts has partial lack of comments

Github Permalinks:

Natspec @return value

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPair.sol#L183-L192 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairCore.sol#L393 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairCore.sol#L409

Natspec in general

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L98 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPair.sol#L45-L156

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPair.sol#L317-L338

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:

parameter _initData is declared but not used

Github Permalinks:

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/VariableInterestRate.sol#L63

Mitigation:

Remove the code that is not used.

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 Also, state variables order affects to gas in the same way as ordering structs for saving storage slots

Details

Modifiers, struct and events are disordered. They are not expected to be found between functions, this hardly affects what the reader is expecting from the code.

github permalink

Events and modifiers https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L50-L1149

Events https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L194-L301

Unexpected struct and events https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L49-L139

Events https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L42-L85

Mitigation

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

Unused named returns

summary

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

github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L393-L404 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L409-L423 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L516-L520 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L201 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L46-L48 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/VariableInterestRate.sol#L52-L54

mitigation

Remove return or returns when both used

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0xA5DF, 0xDjango, 0xNazgul, 0xSmartContract, 0xackermann, 0xbepresent, 0xc0ffEE, 0xkatana, 2997ms, Amithuddar, Aymen0909, Bnke0x0, Chinmay, Chom, CodingNameKiki, Deivitto, Diraco, Dravee, ElKu, EthLedger, Fitraldys, Funen, IgnacioB, JC, Junnon, Lambda, LeoS, Metatron, MiloTruck, Noah3o6, NoamYakov, PaludoX0, Randyyy, ReyAdmirado, Rohan16, Rolezn, Ruhum, SaharAP, Sm4rty, SooYa, TomJ, Tomio, Waze, Yiko, _Adam, __141345__, a12jmx, ajtra, ak1, asutorufos, ballx, brgltd, c3phas, cRat1st0s, carlitox477, chrisdior4, d3e4, delfin454000, dharma09, djxploit, durianSausage, erictee, fatherOfBlocks, find_a_bug, flyx, francoHacker, gerdusx, gogo, gzeon, hakerbaya, ignacio, jag, kyteg, ladboy233, ltyu, m_Rassska, medikko, mics, mrpathfindr, newfork01, nxrblsrpr, oyc_109, pfapostol, rbserver, reassor, ret2basic, robee, sach1r0, saian, simon135, sryysryy, zeesaw

Awards

84.5476 USDC - $84.55

Labels

bug

G (Gas Optimization)

old-submission-method

External Links

Link to GitHub issue

GAS

Public function visibility can be made external

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

https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L74-L76 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L78 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L84 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L89 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L100

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

mitigation

replace each error message in a require by a custom error

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-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L66

mitigation

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

splitting require() statements that use && saves gas

Summary

github permalinks

https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L57-L60 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L61-L64 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L65-L68

mitigation

Instead of using the && operator in a single require statement to check multiple conditions. Consider using multiple require statements with 1 condition per require statement (saving 3 gas per & ):

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

mitigation

Consider shortening the revert strings to fit in 32 bytes

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

Over the next files there are multiple instances of bools https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L78 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L94 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L32-L38 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L52-L53 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/VaultAccount.sol#L18

mitigation

Consider using uint256 with values 0 and 1 rather than bool

Pack state variables tightly

summary

State variables are expected to be ordered by data type, this helps readability and also gas optimization by tightly packing the variables.

github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L51-L137

mitigation

Order in a proper way the state variables to improve readability and to reduce gas usage.

Store using Struct over multiple mappings

summary

All these variables could be combine in a Struct in order to reduce the gas cost.

details

As noticed in: https://gist.github.com/alexon1234/b101e3ac51bea3cbd9cf06f80eaa5bc2 When multiple mappings that access the same addresses, uints, etc, all of them can be mixed into an struct and then that data accessed like: mapping(datatype => newStructCreated) newStructMap; Also, you have this post where it explains the benefits of using Structs over mappings https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e

github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L127-L130

I'm not sure if this mapping is also all_users(address) -> value, if it is, then it can be combined in the same way https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L132-L137

Same doubt here, if the address array is expect to be the same array for every mapping, then the values can be combined into a struct https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L32-L38

mitigation

Consider mixing different mappings into an struct when able in order to save gas.

Make constant state variables that do not change

summary

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

github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L51 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/contracts/FraxlendPairDeployer.sol#L49 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/contracts/FraxlendPairDeployer.sol#L50 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/contracts/FraxlendPairDeployer.sol#L51

mitigation

Add constant to state variables that do not change

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-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L57-L59

mitigation

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

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-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairConstants.sol#L47 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L33 https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L22

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

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

In while loops also applies https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L24

Also for consistency over the code i = i + 1; to ++i; https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L402-L410

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

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-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L289-295 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L308-L314 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L265-L267 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L270-L272 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L51-L54 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L66-L69 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L81-L84

mitigation

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

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

_lenders[i] https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L289-295

_borrowers[i] https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L308-L314

_addresses[i] https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L127-L132 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L152-L160 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L402-L410 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L51-L54 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L66-L69 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L81-L84

data[i] cached and used within a break for the condition to be in the loop https://github.com/code-423n4/2022-08-frax/blob/eaf6d1422ff1f64393a9cacde88f42661b750bfe/src/contracts/libraries/SafeERC20.sol#L27-L29

mitigation

Cache variables used more than one into a local variable.

abi.encode() is less gas efficient than abi.encodePacked()

Summary

In general, abi.encodePacked is more gas-efficient.

Details

Changing the abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient.

Github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L450-L455 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L331 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L374 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L47 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/VariableInterestRate.sol#L53

mitigation

Change abi.encode to abi.encodePacked

Functions guaranteed to revert when called by normal users can be marked payable

Summary

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.

Details

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

Github permalinks

https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L204-L207 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L234-L263 https://github.com/code-423n4/2022-08-frax/blob/d968f256462469239ec7394171409ed76dab03e1/src/contracts/FraxlendPair.sol#L274-L277 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L170-L177 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L50-L55 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L65-L70 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L80-L85

Mitigation

It's suggested to add payable to functions guaranteed to revert when called by normal users to improve gas costs

>= cheaper than >

Summary

Strict inequalities ( > ) are more expensive than non-strict ones ( >= ). This is due to some supplementary checks (ISZERO, 3 gas)

Github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L477 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L754 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L835 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1002 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1094 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L379-L380 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/LinearInterestRate.sol#L66

mitigation

Consider using >= 1 instead of > 0 to avoid some opcodes

<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

Github permalinks

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L724 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L772 https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L773

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-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L1067 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L310 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L356-L363 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L398

Mitigation

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

Extra declaration in named returns that don't use them

summary

Using both named returns and a return statement isn’t necessary, this consumes extra gas as one more variable that is not used is declared .

github permalinks

mitigation

Remove return or returns as needed.

#0 - gititGoro
2022-10-10T00:40:33Z

Good report. No invalid items. Points left on the table:

Gas before and after comparisons
Friendlier formatting such as a table of contents.

#1 - IllIllI000
2022-10-11T21:51:23Z

Good report. No invalid items. Points left on the table:

Gas before and after comparisons

Friendlier formatting such as a table of contents.

@gititGoro The following items are invalid:

Public function visibility can be made external
Explicit initialization
Index initialized in for loop
abi.encode() is less gas efficient than abi.encodePacked()
Extra declaration in named returns that don't use them

Fraxlend (Frax Finance) contest - Deivitto's results

Fraxlend: A permissionless lending platform and the final piece of the Frax Finance Defi Trinity.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-33] QA Report - $99.42

Findings Information

Awards

Labels

External Links

QA

Low

approve should be replaced with safeApprove or safeIncreaseAllowance() / safeDecreaseAllowance()

Summary

Github permalink

Mitigation

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

Summary

Github Permalinks

Mitigation

Missing checks for address(0x0) in withdraw functions

Summary

Details

Github Permalinks

Mitigation

Strict equalities are dangerous in manipulable values

Summary:

Details:

Github Permalinks:

Mitigation:

block.timestamp used as time proxy

Summary:

Details:

References

Github Permalinks:

Mitigation:

Front run initializer

Summary

Details

Github Permalinks

Mitigation

Naming convention of state variable non constant

Summary:

Github Permalinks:

Mitigation

abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccack256()

Summary

Details

Github permalinks

mitigation

Informational

Use of magic numbers is confusing and risky

Summary:

Details:

Github Permalinks:

Mitigation:

Missing indexed event parameters

Summary:

Details:

Github Permalinks:

Mitigation:

Use of string.concat() rather than abi.encodePacked(<str>, <str>)

Summary

Details

Github Permalinks

Mitigation

Implementation of the comment is confusing

Summary:

Details

Github Permalinks:

Mitigation:

Missing Natspec

Summary:

Details:

Github Permalinks:

Natspec @return value

Natspec in general

More documentation needed

mitigation