Fraxlend (Frax Finance) contest - Dravee's results

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Known Issue: Chainlink's latestRoundData might return stale or incorrect results
  • 1.2. Known vulnerabilities exist in currently used @openzeppelin/contracts version
  • 1.3. Unsafe casting may overflow
  • 1.4. Deprecated approve() function
  • 1.5. Missing address(0) checks
  • 1.6. deployedPairsArray is an unbounded array, this could lead to DoS
  • 1.7. Prevent accidentally burning tokens
  • 1.8. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. The nonReentrant modifier should occur before all other modifiers
  • 2.2. Typos
  • 2.3. Related mappings should be grouped in a struct
  • 2.4. Use string.concat() or bytes.concat()
  • 2.5. Adding a return statement when the function defines a named return variable, is redundant
  • 2.6. Non-library/interface files should use fixed compiler versions, not floating ones

1. Low Risk Issues

1.1. Known Issue: Chainlink's latestRoundData might return stale or incorrect results

Even as a known issue, the mitigation still has its place in a QA Report.

latestRoundData() is used to fetch the asset price from a Chainlink aggregator, but it's missing additional validations to ensure that the round is complete. If there is a problem with Chainlink starting a new round and finding consensus on the new value for the oracle (e.g. Chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the Chainlink system) consumers of this contract may continue using outdated stale data / stale prices.

Consider adding missing checks for stale data here:

File: FraxlendPairCore.sol
523:         if (oracleMultiply != address(0)) {
- 524:             (, int256 _answer, , , ) = AggregatorV3Interface(oracleMultiply).latestRoundData();
+ 524:             (uint80 roundID, int256 _answer, , uint256 updatedAt, uint80 answeredInRound) = AggregatorV3Interface(oracleMultiply).latestRoundData();
+ 524:             require(updatedAt > 0, "Price data is stale");
+ 524:             require(answeredInRound >= roundID, "Price data is stale");
525:             if (_answer <= 0) {
526:                 revert OracleLTEZero(oracleMultiply);
527:             }
528:             _price = _price * uint256(_answer);
529:         }
530: 
531:         if (oracleDivide != address(0)) {
- 532:             (, int256 _answer, , , ) = AggregatorV3Interface(oracleDivide).latestRoundData();
+ 532:             (uint80 roundID, int256 _answer, , uint256 updatedAt, uint80 answeredInRound) = AggregatorV3Interface(oracleDivide).latestRoundData();
+ 532:             require(updatedAt > 0, "Price data is stale");
+ 532:             require(answeredInRound >= roundID, "Price data is stale");
533:             if (_answer <= 0) {
534:                 revert OracleLTEZero(oracleDivide);
535:             }
536:             _price = _price / uint256(_answer);
537:         }

1.2. Known vulnerabilities exist in currently used @openzeppelin/contracts version

As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/contracts@4.7.3 here:

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/package.json#L23

    "@openzeppelin/contracts": "^4.7.2",

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)

1.3. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting.

FraxlendPair.sol:252:        _totalAsset.amount -= uint128(_amountToTransfer);
FraxlendPairCore.sol:475:                _totalBorrow.amount += uint128(_interestEarned);
FraxlendPairCore.sol:476:                _totalAsset.amount += uint128(_interestEarned);
FraxlendPairCore.sol:484:                    _totalAsset.shares += uint128(_feesShare);
FraxlendPairCore.sol:543:        _exchangeRateInfo.exchangeRate = uint224(_exchangeRate);
FraxlendPairCore.sol:544:        _exchangeRateInfo.lastTimestamp = uint32(block.timestamp);
FraxlendPairCore.sol:719:        _totalBorrow.shares += uint128(_sharesAdded);
LinearInterestRate.sol:85:            _newRatePerSec = uint64(_minInterest + ((_utilization * _slope) / UTIL_PREC));
LinearInterestRate.sol:90:            _newRatePerSec = uint64(_vertexInterest);
VariableInterestRate.sol:71:            _newRatePerSec = uint64((_currentRatePerSec * INT_HALF_LIFE) / _decayGrowth);
VariableInterestRate.sol:78:            _newRatePerSec = uint64((_currentRatePerSec * _decayGrowth) / INT_HALF_LIFE);

Consider casting like it's done here:

FraxlendPairCore.sol:613:        _deposit(_totalAsset, _amountReceived.toUint128(), _shares.toUint128(), _receiver);
FraxlendPairCore.sol:668:        _redeem(_totalAsset, _amountToReturn.toUint128(), _shares.toUint128(), _receiver, _owner);
FraxlendPairCore.sol:684:        _redeem(_totalAsset, _amount.toUint128(), _shares.toUint128(), _receiver, _owner);
FraxlendPairCore.sol:757:        _shares = _borrowAsset(_borrowAmount.toUint128(), _receiver);
FraxlendPairCore.sol:886:        _repayAsset(_totalBorrow, _amountToRepay.toUint128(), _shares.toUint128(), msg.sender, _borrower);
FraxlendPairCore.sol:937:        _repayAsset(_totalBorrow, _amountToRepay.toUint128(), _shares.toUint128(), msg.sender, _borrower); // liquidator repays shares on behalf of borrower
FraxlendPairCore.sol:967:        uint128 _borrowerShares = userBorrowShares[_borrower].toUint128();
FraxlendPairCore.sol:984:            _leftoverCollateral = (_userCollateralBalance.toInt256() - _optimisticCollateralForLiquidator.toInt256());
FraxlendPairCore.sol:993:        uint128 _amountLiquidatorToRepay = (_totalBorrow.toAmount(_sharesToLiquidate, true)).toUint128();
FraxlendPairCore.sol:1005:                    _amountToAdjust = (_totalBorrow.toAmount(_sharesToAdjust, false)).toUint128();
FraxlendPairCore.sol:1100:        uint256 _borrowShares = _borrowAsset(_borrowAmount.toUint128(), address(this));
FraxlendPairCore.sol:1209:        _repayAsset(_totalBorrow, _amountAssetOut.toUint128(), _sharesToRepay.toUint128(), address(this), msg.sender);

1.4. Deprecated approve() function

While safeApprove() in itself is deprecated, it is still better than approve which is subject to a known front-running attack and failing for certain token implementations that do not return a boolean value. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

FraxlendPairCore.sol:1103:        _assetContract.approve(_swapperAddress, _borrowAmount);
FraxlendPairCore.sol:1184:        _collateralContract.approve(_swapperAddress, _collateralToSwap);

1.5. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

FraxlendPairCore.sol:58:    IERC20 internal immutable assetContract;
FraxlendPairCore.sol:59:    IERC20 public immutable collateralContract;
FraxlendPairCore.sol:62:    address public immutable oracleMultiply;
FraxlendPairCore.sol:63:    address public immutable oracleDivide;
FraxlendPairCore.sol:74:    IRateCalculator public immutable rateContract; // For complex rate calculations
FraxlendPairCore.sol:81:    address public immutable DEPLOYER_ADDRESS;
FraxlendPairCore.sol:84:    address public immutable CIRCUIT_BREAKER_ADDRESS;
FraxlendPairCore.sol:85:    address public immutable COMPTROLLER_ADDRESS;
FraxlendPairCore.sol:89:    address public immutable FRAXLEND_WHITELIST_ADDRESS;
FraxlendPairDeployer.sol:62:    address private immutable FRAXLEND_WHITELIST_ADDRESS;

1.6. deployedPairsArray is an unbounded array, this could lead to DoS

As deployedPairsArray can grow quite large (only push operations, no pop, no delete, no subtraction on <array>.length), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

FraxlendPairDeployer.sol:127:        for (i = 0; i < _lengthOfArray; ) {
FraxlendPairDeployer.sol:152:        for (i = 0; i < _lengthOfArray; ) {
FraxlendPairDeployer.sol:255:        deployedPairsArray.push(_name);
FraxlendPairDeployer.sol:402:        for (uint256 i = 0; i < _lengthOfArray; ) {

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

1.7. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

FraxlendPair.sol:261:        assetContract.safeTransfer(_recipient, _amountToTransfer);
FraxlendPairCore.sol:651:        assetContract.safeTransfer(_receiver, _amountToReturn);
FraxlendPairCore.sol:728:            assetContract.safeTransfer(_receiver, _borrowAmount);
FraxlendPairCore.sol:777:            collateralContract.safeTransferFrom(_sender, address(this), _collateralAmount);
FraxlendPairCore.sol:819:            collateralContract.safeTransfer(_receiver, _collateralAmount);
FraxlendPairCore.sol:872:            assetContract.safeTransferFrom(_payer, address(this), _amountToRepay);

1.8. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

FraxlendPairDeployer.sol:29:import "@openzeppelin/contracts/access/Ownable.sol";
FraxlendPairDeployer.sol:44:contract FraxlendPairDeployer is Ownable {
FraxlendPairDeployer.sol:262:        _fraxlendPair.transferOwnership(COMPTROLLER_ADDRESS);

FraxlendWhitelist.sol:28:import "@openzeppelin/contracts/access/Ownable.sol";
FraxlendWhitelist.sol:30:contract FraxlendWhitelist is Ownable {

2. Non-Critical Issues

2.1. The nonReentrant modifier should occur before all other modifiers

This is a best-practice to protect against re-entrancy in other modifiers

• src/contracts/FraxlendPairCore.sol:

   739      function borrowAsset(
   740          uint256 _borrowAmount,
   741          uint256 _collateralAmount,
   742          address _receiver
   743      )
   744          external
   745          isNotPastMaturity
   746          whenNotPaused
   747:         nonReentrant
   748          isSolvent(msg.sender)
   749          approvedBorrower
   750          returns (uint256 _shares)

   911      function liquidate(uint256 _shares, address _borrower)
   912          external
   913          whenNotPaused
   914:         nonReentrant
   915          approvedLender(msg.sender)
   916          returns (uint256 _collateralForLiquidator)

   950      function liquidateClean(
   951          uint128 _sharesToLiquidate,
   952          uint256 _deadline,
   953          address _borrower
   954:     ) external whenNotPaused nonReentrant approvedLender(msg.sender) returns (uint256 _collateralForLiquidator) {

  1062      function leveragedPosition(
  1063          address _swapperAddress,
  1064          uint256 _borrowAmount,
  1065          uint256 _initialCollateralAmount,
  1066          uint256 _amountCollateralOutMin,
  1067          address[] memory _path
  1068      )
  1069          external
  1070          isNotPastMaturity
  1071:         nonReentrant
  1072          whenNotPaused
  1073          approvedBorrower
  1074          isSolvent(msg.sender)
  1075          returns (uint256 _totalCollateralBalance)

2.2. Typos

• toBorrtoBorrowAmountowShares

FraxlendPair.sol:187:    /// @notice The ```toBorrtoBorrowAmountowShares``` function converts a given amount of borrow debt into the number of shares

• whos

FraxlendPair.sol:286:    /// @param _lenders The addresses whos status will be set
FraxlendPair.sol:305:    /// @param _borrowers The addresses whos status will be set

• approcal

FraxlendPair.sol:287:    /// @param _approval The approcal status
FraxlendPair.sol:306:    /// @param _approval The approcal status

• whitlist

FraxlendPairCore.sol:231:        // Set approved lenders whitlist active

• occured

FraxlendPairCore.sol:893:    /// @param _borrower The borrower account for which the liquidation occured

• liabilites

FraxlendPairCore.sol:896:    /// @param _sharesToAdjust The number of Borrow Shares that were adjusted on liabilites and assets (a writeoff)

• Calced

FraxlendPairCore.sol:992:        // Calced here for use during repayment, grouped with other calcs before effects start

• stuct

FraxlendPairCore.sol:1010:                    // Note: Ensure this memory stuct will be passed to _repayAsset for write to state

• accomodate

FraxlendPairDeployer.sol:168:    /// @dev splits the data if necessary to accomodate creation code that is slightly larger than 24kb

• unopinionated

LinearInterestRate.sol:72:    /// @dev We use calldata to remain unopinionated about future implementations

• calulcating

VariableInterestRate.sol:32:/// @notice A Contract for calulcating interest rates as a function of utilization and time

2.3. Related mappings should be grouped in a struct

The "Given address in the DA" mappings should be grouped in a struct.

FraxlendPairCore.sol:127:    mapping(address => uint256) public userCollateralBalance; // amount of collateral each user is backed
FraxlendPairCore.sol:129:    mapping(address => uint256) public userBorrowShares; // represents the shares held by individuals

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete userInfo[address].

2.4. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)) Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

FraxlendPair.sol:2:pragma solidity ^0.8.15;
FraxlendPair.sol:81:        return string(abi.encodePacked("FraxlendV1 - ", collateralContract.safeSymbol(), "/", assetContract.safeSymbol()));

FraxlendPairDeployer.sol:2:pragma solidity ^0.8.15;
FraxlendPairDeployer.sol:204:            bytes32 salt = keccak256(abi.encodePacked(_saltSeed, _configData));
FraxlendPairDeployer.sol:211:             bytes memory bytecode = abi.encodePacked(
FraxlendPairDeployer.sol:212:                 _creationCode,
FraxlendPairDeployer.sol:213:                 abi.encode(
FraxlendPairDeployer.sol:214:                     _configData,
FraxlendPairDeployer.sol:215:                     _immutables,
FraxlendPairDeployer.sol:216:                     _maxLTV,
FraxlendPairDeployer.sol:217:                     _liquidationFee,
FraxlendPairDeployer.sol:218:                     _maturityDate,
FraxlendPairDeployer.sol:219:                     _penaltyRate,
FraxlendPairDeployer.sol:220:                     _isBorrowerWhitelistActive,
FraxlendPairDeployer.sol:221:                     _isLenderWhitelistActive
FraxlendPairDeployer.sol:222:                 )
FraxlendPairDeployer.sol:223:             );

2.5. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

• File: LinearInterestRate.sol

46:     function getConstants() external pure returns (bytes memory _calldata) {
47:         return abi.encode(MIN_INT, MAX_INT, MAX_VERTEX_UTIL, UTIL_PREC);
48:     }

• File: VariableInterestRate.sol

52:     function getConstants() external pure returns (bytes memory _calldata) {
53:         return abi.encode(MIN_UTIL, MAX_UTIL, UTIL_PREC, MIN_INT, MAX_INT, INT_HALF_LIFE);
54:     }

2.6. Non-library/interface files should use fixed compiler versions, not floating ones

FraxlendPair.sol:2:pragma solidity ^0.8.15;
FraxlendPairConstants.sol:2:pragma solidity ^0.8.15;
FraxlendPairCore.sol:2:pragma solidity ^0.8.15;
FraxlendPairDeployer.sol:2:pragma solidity ^0.8.15;
FraxlendWhitelist.sol:2:pragma solidity ^0.8.15;
LinearInterestRate.sol:2:pragma solidity ^0.8.15;
VariableInterestRate.sol:2:pragma solidity ^0.8.15;

Overview

Table of Contents:

• 1. State variables only set in the constructor should be declared immutable
• 2. Multiple accesses of a mapping/array should use a local variable cache
• 3. Caching storage values in memory
• 4. Use calldata instead of memory
• 5. Reduce the size of error messages (Long revert Strings)
• 6. Multiple address mappings can be combined in a struct
• 7. Splitting require() statements that use && saves gas
• 8. <array>.length should not be looked up in every loop of a for-loop
• 9. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 10. Increments/decrements can be unchecked in for-loops
• 11. It costs more gas to initialize variables with their default value than letting the default value be applied
• 12. Use Custom Errors instead of Revert Strings to save Gas
• 13. (Not recommended, but true) Functions guaranteed to revert when called by normal users can be marked payable

1. State variables only set in the constructor should be declared immutable

Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)

FraxlendPairDeployer.sol:57:    address public CIRCUIT_BREAKER_ADDRESS;
FraxlendPairDeployer.sol:58:    address public COMPTROLLER_ADDRESS;
FraxlendPairDeployer.sol:59:    address public TIME_LOCK_ADDRESS;

2. Multiple accesses of a mapping/array should use a local variable cache

Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.

Affected code:

• _addresses[i] (calldata)

FraxlendPairDeployer.sol:154:                _address: _addresses[i],
FraxlendPairDeployer.sol:155:                _isCustom: deployedPairCustomStatusByAddress[_addresses[i]]

FraxlendWhitelist.sol:52:            oracleContractWhitelist[_addresses[i]] = _bool;
FraxlendWhitelist.sol:53:            emit SetOracleWhitelist(_addresses[i], _bool);

FraxlendWhitelist.sol:67:            rateContractWhitelist[_addresses[i]] = _bool;
FraxlendWhitelist.sol:68:            emit SetRateContractWhitelist(_addresses[i], _bool);

FraxlendWhitelist.sol:82:            fraxlendDeployerWhitelist[_addresses[i]] = _bool;
FraxlendWhitelist.sol:83:            emit SetFraxlendDeployerWhitelist(_addresses[i], _bool);

• _lenders[i] (calldata)

FraxlendPair.sol:291:            if (_approval || _lenders[i] != msg.sender) {
FraxlendPair.sol:292:                approvedLenders[_lenders[i]] = _approval;
FraxlendPair.sol:293:                emit SetApprovedLender(_lenders[i], _approval);

• _borrowers[i] (calldata)

FraxlendPair.sol:310:            if (_approval || _borrowers[i] != msg.sender) {
FraxlendPair.sol:311:                approvedBorrowers[_borrowers[i]] = _approval;
FraxlendPair.sol:312:                emit SetApprovedBorrower(_borrowers[i], _approval);

3. Caching storage values in memory

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Here, we can save 1 SLOAD for reading DEFAULT_MAX_LTV and 1 SLOAD for reading DEFAULT_LIQ_FEE

src/contracts/FraxlendPairDeployer.sol:
  332:             DEFAULT_MAX_LTV, //@audit SLOAD 1 (DEFAULT_MAX_LTV)
  333:             DEFAULT_LIQ_FEE,//@audit SLOAD 1 (DEFAULT_LIQ_FEE)
  342:         _logDeploy(_name, _pairAddress, _configData, DEFAULT_MAX_LTV, DEFAULT_LIQ_FEE, 0);//@audit SLOAD 2 (DEFAULT_MAX_LTV) and SLOAD 2 (DEFAULT_LIQ_FEE)

4. Use calldata instead of memory

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly bypasses this loop.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gas-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Affected code (around 60 gas per occurrence):

FraxlendPairDeployer.sol:310:    function deploy(bytes memory _configData) external returns (address _pairAddress) {
FraxlendPairDeployer.sol:398:    function globalPause(address[] memory _addresses) external returns (address[] memory _updatedAddresses) {

5. Reduce the size of error messages (Long revert Strings)

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.

Revert strings > 32 bytes:

FraxlendPairDeployer.sol:205:            require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");
FraxlendPairDeployer.sol:228:            require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");
FraxlendPairDeployer.sol:253:        require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");
FraxlendPairDeployer.sol:365:        require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");
FraxlendPairDeployer.sol:368:            "FraxlendPairDeployer: Only whitelisted addresses"
LinearInterestRate.sol:59:            "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
LinearInterestRate.sol:63:            "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
LinearInterestRate.sol:67:            "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"

Consider shortening the revert strings to fit in 32 bytes.

6. Multiple address mappings can be combined in a struct

Computing storage costs ~42 gas, and this could be saved per access due to not having to recalculate the key's keccak256 hash.

FraxlendPairCore.sol:127:    mapping(address => uint256) public userCollateralBalance; // amount of collateral each user is backed
FraxlendPairCore.sol:129:    mapping(address => uint256) public userBorrowShares; // represents the shares held by individuals

7. Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

Affected code (saving around 3 gas per instance):

LinearInterestRate.sol:58:            _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
LinearInterestRate.sol:62:            _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
LinearInterestRate.sol:66:            _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,

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

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

FraxlendPair.sol:289:        for (uint256 i = 0; i < _lenders.length; i++) {
FraxlendPair.sol:308:        for (uint256 i = 0; i < _borrowers.length; i++) {
FraxlendPairCore.sol:265:        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
FraxlendPairCore.sol:270:        for (uint256 i = 0; i < _approvedLenders.length; ++i) {
FraxlendWhitelist.sol:51:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:66:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:81:        for (uint256 i = 0; i < _addresses.length; i++) {

9. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

libraries/SafeERC20.sol:24:                i++;
libraries/SafeERC20.sol:27:            for (i = 0; i < 32 && data[i] != 0; i++) {
FraxlendPair.sol:289:        for (uint256 i = 0; i < _lenders.length; i++) {
FraxlendPair.sol:308:        for (uint256 i = 0; i < _borrowers.length; i++) {
FraxlendPairDeployer.sol:130:                i++;
FraxlendPairDeployer.sol:158:                i++;
FraxlendWhitelist.sol:51:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:66:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:81:        for (uint256 i = 0; i < _addresses.length; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

10. Increments/decrements can be unchecked in for-loops

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.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

libraries/SafeERC20.sol:27:            for (i = 0; i < 32 && data[i] != 0; i++) {
FraxlendPair.sol:289:        for (uint256 i = 0; i < _lenders.length; i++) {
FraxlendPair.sol:308:        for (uint256 i = 0; i < _borrowers.length; i++) {
FraxlendPairCore.sol:265:        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
FraxlendPairCore.sol:270:        for (uint256 i = 0; i < _approvedLenders.length; ++i) {
FraxlendWhitelist.sol:51:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:66:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:81:        for (uint256 i = 0; i < _addresses.length; i++) {

The change would be:

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

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existent for uint256 here.

11. It costs more gas to initialize variables with their default value than letting the default value be applied

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...). Explicitly initializing it with its default value is an anti-pattern and wastes gas (around 3 gas per instance).

Affected code:

libraries/SafeERC20.sol:22:            uint8 i = 0;
FraxlendPair.sol:289:        for (uint256 i = 0; i < _lenders.length; i++) {
FraxlendPair.sol:308:        for (uint256 i = 0; i < _borrowers.length; i++) {
FraxlendPairCore.sol:265:        for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
FraxlendPairCore.sol:270:        for (uint256 i = 0; i < _approvedLenders.length; ++i) {
FraxlendPairDeployer.sol:402:        for (uint256 i = 0; i < _lengthOfArray; ) {
FraxlendWhitelist.sol:51:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:66:        for (uint256 i = 0; i < _addresses.length; i++) {
FraxlendWhitelist.sol:81:        for (uint256 i = 0; i < _addresses.length; i++) {

Consider removing explicit initializations for default values.

12. Use Custom Errors instead of Revert Strings to save Gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Additionally, custom errors can be used inside and outside of contracts (including interfaces and libraries).

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences:

FraxlendPairDeployer.sol:205:            require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");
FraxlendPairDeployer.sol:228:            require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");
FraxlendPairDeployer.sol:253:        require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");
FraxlendPairDeployer.sol:365:        require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");
FraxlendPairDeployer.sol:366:        require(
FraxlendPairDeployer.sol:399:        require(msg.sender == CIRCUIT_BREAKER_ADDRESS, "Circuit Breaker only");
LinearInterestRate.sol:57:        require(
LinearInterestRate.sol:61:        require(
LinearInterestRate.sol:65:        require(

13. (Not recommended, but true) Functions guaranteed to revert when called by normal users can be marked payable

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.

FraxlendPair.sol:204:    function setTimeLock(address _newAddress) external onlyOwner {
FraxlendPair.sol:234:    function withdrawFees(uint128 _shares, address _recipient) external onlyOwner returns (uint256 _amountToTransfer) {
FraxlendPair.sol:274:    function setSwapper(address _swapper, bool _approval) external onlyOwner {
FraxlendPairDeployer.sol:170:    function setCreationCode(bytes calldata _creationCode) external onlyOwner {
FraxlendWhitelist.sol:50:    function setOracleContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
FraxlendWhitelist.sol:65:    function setRateContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
FraxlendWhitelist.sol:80:    function setFraxlendDeployerWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {