Fraxlend (Frax Finance) contest - pfapostol's results

Summary

Low Risk Issues

Non-critical Issues

Total: 38 instances over 11 issues

Low Risk:

1. Missing checks for address(0x0) when assigning values to address state variables (9 instances)

   Zero-address checks are a best practice for input validation of critical address parameters. Accidental use of zero-addresses may result in exceptions, burn fees/tokens.

   • src/contracts/FraxlendPairDeployer.sol:104-107

   104     CIRCUIT_BREAKER_ADDRESS = _circuitBreaker;
   105     COMPTROLLER_ADDRESS = _comptroller;
   106     TIME_LOCK_ADDRESS = _timelock;
   107     FRAXLEND_WHITELIST_ADDRESS = _fraxlendWhitelist;

   • src/contracts/FraxlendPairCore.sol:172-175

   172     CIRCUIT_BREAKER_ADDRESS = _circuitBreaker;
   173     COMPTROLLER_ADDRESS = _comptrollerAddress;
   174     TIME_LOCK_ADDRESS = _timeLockAddress;
   175     FRAXLEND_WHITELIST_ADDRESS = _fraxlendWhitelistAddress;

   • src/contracts/FraxlendPair.sol:206

   206     TIME_LOCK_ADDRESS = _newAddress;

Non-Critical:

1. approve return values not checked (2 instances)

   I chose not critical as the transaction will fail if the approval is invalid. But it can be considered as low, since the user will spend significantly more when calling an external function than if it were reverted immediately.

   • contracts/contracts/FraxlendPairCore.sol:1103,1184

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

   fix:

   if (!_assetContract.approve(_swapperAddress, _borrowAmount)) revert NotApproved();

   OR: use safeApprove (just change function, because _assetContract and _collateralContract is already using SafeERC20)

2. Double check for overflow (3 instances)

   Arithmetic operations revert on underflow and overflow. You can use unchecked { ... } to use the previous wrapping behaviour.

   • contracts/contracts/FraxlendPairCore.sol:471-476,542-543,633

   471     if (
   472         _interestEarned + _totalBorrow.amount <= type(uint128).max &&
   473         _interestEarned + _totalAsset.amount <= type(uint128).max
   474     ) {
   475         _totalBorrow.amount += uint128(_interestEarned);
   476         _totalAsset.amount += uint128(_interestEarned);
   ...
   542     if (_exchangeRate > type(uint224).max) revert PriceTooLarge();
   543     _exchangeRateInfo.exchangeRate = uint224(_exchangeRate);
   ...
   633     if (allowed != type(uint256).max) _approve(_owner, msg.sender, allowed - _shares);

   fix:

   1. Use unchecked if you check code, or
   2. remove overflow check

   Both fixes save gas.

3. constants should be defined rather than using magic numbers (7 instances)

   • contracts/contracts/FraxlendPairCore.sol:468,522

   468     _interestEarned = (_deltaTime * _totalBorrow.amount * _currentRateInfo.ratePerSec) / 1e18;
   ...
   522     uint256 _price = uint256(1e36);

   • contracts/contracts/VariableInterestRate.sol:69,76

   69      uint256 _deltaUtilization = ((MIN_UTIL - _utilization) * 1e18) / MIN_UTIL;
   ...
   76      uint256 _deltaUtilization = ((_utilization - MAX_UTIL) * 1e18) / (UTIL_PREC - MAX_UTIL);

   • contracts/contracts/FraxlendPairDeployer.sol:171,173-174

   171     bytes memory _firstHalf = BytesLib.slice(_creationCode, 0, 13000);
   ...
   173     if (_creationCode.length > 13000) {
   174         bytes memory _secondHalf = BytesLib.slice(_creationCode, 13000, _creationCode.length - 13000);

4. public functions not called by the contract should be declared external instead (5 instances)

   Contracts are allowed to override their parents' functions and change the visibility from external to public.

   • contracts/contracts/FraxlendPair.sol:74,78,84,89,100,

   74      function name() public view override(ERC20, IERC20Metadata) returns (string memory) {
   ...
   78      function symbol() public view override(ERC20, IERC20Metadata) returns (string memory) {
   ...
   84      function decimals() public pure override(ERC20, IERC20Metadata) returns (uint8) {
   ...
   89      function totalSupply() public view override(ERC20, IERC20) returns (uint256) {
   ...
   100     function totalAssets() public view virtual returns (uint256) {

5. Unused Parameter (1 instances)

   Removing unused named return variables can reduce gas usage and improve code clarity.

   • src/contracts/VariableInterestRate.sol:63

   63      function getNewRate(bytes calldata _data, bytes calldata _initData)

6. Unused Named Return (1 instances)

   return is redundant when using named returns

   • src/contracts/FraxlendPairCore.sol:519

   519     return _exchangeRate = _exchangeRateInfo.exchangeRate;

7. Function parameter missing from NatSpec (1 instances)

   Makes it difficult for the user to understand the parameter

   • src/contracts/FraxlendPairCore.sol:153

   153     bytes memory _immutables,

8. Redundant type cast uint256 (1 instances)

   • src/contracts/FraxlendPairCore.sol:522

   522     uint256 _price = uint256(1e36);

9. Two contracts the similar names (1 instances)

   Detected with: slither - https://github.com/crytic/slither/wiki/Detector-Documentation#name-reused

   If a codebase has two contracts the similar names, the compilation artifacts will not contain one of the contracts with the duplicate name.

   SafeERC20 is re-used:

   • SafeERC20 (node_modules/@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol#19-116)
   • SafeERC20 (contracts/contracts/libraries/SafeERC20.sol#13-76)

10. Non-library/interface files should use fixed compiler versions, not floating ones (7 instances)

    • 2022-08-frax/src/contracts/FraxlendPair.sol

    • 2022-08-frax/src/contracts/FraxlendPairConstants.sol

    • 2022-08-frax/src/contracts/FraxlendPairCore.sol

    • 2022-08-frax/src/contracts/FraxlendPairDeployer.sol

    • 2022-08-frax/src/contracts/FraxlendWhitelist.sol

    • 2022-08-frax/src/contracts/LinearInterestRate.sol

    • 2022-08-frax/src/contracts/VariableInterestRate.sol

         pragma solidity ^0.8.15;

Summary

Gas savings are estimated using gas report of existing tests source .env && forge test --fork-url $MAINNET_URL --fork-block-number $DEFAULT_FORK_BLOCK --gas-report (the sum of all deployment costs and the sum of the cost of calling all methods) and may vary depending on the implementation of the fix. I keep my version of the fix for each finding and can provide them if you need. The accuracy of the estimate depends on the test coverage. I'm not "foundry-friendly", and unfortunately I didn't have enough time to learn how to write additional tests.

Gas Optimizations

Total: 100 instances over 11 issues

1. Error handling

   1. Use Custom Errors instead of Revert/Require Strings to save Gas (9 instances)

      Deployment Gas Saved: 155360

      Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

      Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Additional gas is saved due to the lack of defining string. https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth

      ---

      • src/contracts/LinearInterestRate.sol:57-68

      57      require(
      58          _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
      59          "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
      60      );
      61      require(
      62          _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
      63          "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
      64      );
      65      require(
      66          _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
      67          "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
      68      );

      • src/contracts/FraxlendPairDeployer.sol:205,228,253,365-369,399,

      205     require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");
      ...
      228     require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");
      ...
      253     require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");
      ...
      365     require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");
      366     require(
      367         IFraxlendWhitelist(FRAXLEND_WHITELIST_ADDRESS).fraxlendDeployerWhitelist(msg.sender),
      368         "FraxlendPairDeployer: Only whitelisted addresses"
      369     );
      ...
      399     require(msg.sender == CIRCUIT_BREAKER_ADDRESS, "Circuit Breaker only");

   2. Require()/revert() strings longer than 32 bytes cost extra gas (8 instances)

      If you opt not to use custom errors keeping require strings <= 32 bytes in length will save gas.

      Deployment Gas Saved: 70073

      • src/contracts/LinearInterestRate.sol:57-68

      57      require(
      58          _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
      59          "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
      60      );
      61      require(
      62          _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
      63          "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
      64      );
      65      require(
      66          _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
      67          "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
      68      );

      • src/contracts/FraxlendPairDeployer.sol:205,228,253,365-369,399,

      205     require(deployedPairsBySalt[salt] == address(0), "FraxlendPairDeployer: Pair already deployed");
      ...
      228     require(_pairAddress != address(0), "FraxlendPairDeployer: create2 failed");
      ...
      253     require(deployedPairsByName[_name] == address(0), "FraxlendPairDeployer: Pair name must be unique");
      ...
      365     require(_maxLTV <= GLOBAL_MAX_LTV, "FraxlendPairDeployer: _maxLTV is too large");
      366     require(
      367         IFraxlendWhitelist(FRAXLEND_WHITELIST_ADDRESS).fraxlendDeployerWhitelist(msg.sender),
      368         "FraxlendPairDeployer: Only whitelisted addresses"
      369     );

   3. Splitting require() statements that use && saves gas (3 instances)

      Optional, if you do not want to use model if () revert CustomError(); Costs extra gas to deploy, but saves gas in the long run due to cheaper method calls

      • src/contracts/LinearInterestRate.sol:57-68

      57      require(
      58          _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
      59          "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
      60      );
      61      require(
      62          _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
      63          "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
      64      );
      65      require(
      66          _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
      67          "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
      68      );

2. Variable can be a constant (4 instances)

   • src/contracts/FraxlendPairDeployer.sol:49-51

   Deployment Gas Saved: 108477 Method Call Gas Saved: ≈66179

   48      // Constants
   49      uint256 public DEFAULT_MAX_LTV = 75000; // 75% with 1e5 precision
   50      uint256 public GLOBAL_MAX_LTV = 1e8; // 1000x (100,000%) with 1e5 precision, protects from rounding errors in LTV calc
   51      uint256 public DEFAULT_LIQ_FEE = 10000; // 10% with 1e5 precision

   fix:

   49      uint256 private constant DEFAULT_MAX_LTV = 75000; // 75% with 1e5 precision
   50      uint256 private constant GLOBAL_MAX_LTV = 1e8; // 1000x (100,000%) with 1e5 precision, protects from rounding errors in LTV calc
   51      uint256 private constant DEFAULT_LIQ_FEE = 10000; // 10% with 1e5 precision

   • src/contracts/FraxlendPairCore.sol:51

   51      string public version = "1.0.0";

   fix:

   51      string private constant version = "1.0.0";

3. Increment optimization.

   1. Prefix increments are cheaper than postfix increments, especially when it's used in for-loops (9 instances).

      Deployment Gas Saved: 3613 Method Call Gas Saved: ≈6305

      • src/contracts/libraries/SafeERC20.sol:24,27

      24      i++;
      ...
      27      for (i = 0; i < 32 && data[i] != 0; i++) {

      • src/contracts/FraxlendPairDeployer.sol:130,158

      130      i++;
      ...
      158      i++;

      • src/contracts/FraxlendWhitelist.sol:51,66,81

      51      for (uint256 i = 0; i < _addresses.length; i++) {
      ...
      66      for (uint256 i = 0; i < _addresses.length; i++) {
      ...
      81      for (uint256 i = 0; i < _addresses.length; i++) {

      • src/contracts/FraxlendPair.sol:289,308

      289     for (uint256 i = 0; i < _lenders.length; i++) {
      ...
      308     for (uint256 i = 0; i < _borrowers.length; i++) {

   2. <x> = <x> + 1 even more efficient than pre increment.(11 instances) Gas saved:

      Deployment Gas Saved: 6607 Method Call Gas Saved: ≈14176

      • src/contracts/libraries/SafeERC20.sol:24,27

      24      i++;
      ...
      27      for (i = 0; i < 32 && data[i] != 0; i++) {

      • src/contracts/FraxlendPairDeployer.sol:130,158

      130      i++;
      ...
      158      i++;

      • src/contracts/FraxlendWhitelist.sol:51,66,81

      51      for (uint256 i = 0; i < _addresses.length; i++) {
      ...
      66      for (uint256 i = 0; i < _addresses.length; i++) {
      ...
      81      for (uint256 i = 0; i < _addresses.length; i++) {

      • src/contracts/FraxlendPair.sol:289,308

      289     for (uint256 i = 0; i < _lenders.length; i++) {
      ...
      308     for (uint256 i = 0; i < _borrowers.length; i++) {

      • src/contracts/FraxlendPairCore.sol:265,270

      265     for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
      ...
      270     for (uint256 i = 0; i < _approvedLenders.length; ++i) {

   3. Expression can be unchecked when overflow is not possible.(11 instances) Gas saved:

      Deployment Gas Saved: 45459 Method Call Gas Saved: ≈59563

      • src/contracts/libraries/SafeERC20.sol:24,27

      24      i++;
      ...
      27      for (i = 0; i < 32 && data[i] != 0; i++) {

      • src/contracts/FraxlendWhitelist.sol:51,66,81

      51      for (uint256 i = 0; i < _addresses.length; i++) {
      ...
      66      for (uint256 i = 0; i < _addresses.length; i++) {
      ...
      81      for (uint256 i = 0; i < _addresses.length; i++) {

      • src/contracts/FraxlendPair.sol:289,308

      289     for (uint256 i = 0; i < _lenders.length; i++) {
      ...
      308     for (uint256 i = 0; i < _borrowers.length; i++) {

      • src/contracts/FraxlendPairCore.sol:265,270

      265     for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
      ...
      270     for (uint256 i = 0; i < _approvedLenders.length; ++i) {

      • src/contracts/libraries/VaultAccount.sol:26,43

      26      shares = shares + 1; // @note Theoretically, it cannot overflow, because the result of division in 24
      ...
      43      amount = amount + 1; // @note Theoretically, it cannot overflow, because the result of division in 41

4. State variables only set in the constructor should be declared immutable (3 instances)

   Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

   Deployment Gas Saved: 8352 Method Call Gas Saved: ≈1841

   • src/contracts/FraxlendPairDeployer.sol:57-59

   57      address public CIRCUIT_BREAKER_ADDRESS;
   58      address public COMPTROLLER_ADDRESS;
   59      address public TIME_LOCK_ADDRESS;

5. x = x + y is more efficient than x += y (22 instances)

   Deployment Gas Saved: 8009 Method Call Gas Saved: ≈22006

   • src/contracts/FraxlendPairCore.sol:475-476,484,566-567,643-644,718-719,724,772-773,813,815,863-864,867,1008,1011-1012

   475     _totalBorrow.amount += uint128(_interestEarned);
   476     _totalAsset.amount += uint128(_interestEarned);
   ...
   484     _totalAsset.shares += uint128(_feesShare);
   ...
   566     _totalAsset.amount += _amount;
   567     _totalAsset.shares += _shares;
   ...
   643     _totalAsset.amount -= _amountToReturn;
   644     _totalAsset.shares -= _shares;
   ...
   718     _totalBorrow.amount += _borrowAmount;
   719     _totalBorrow.shares += uint128(_sharesAdded);
   ...
   724     userBorrowShares[msg.sender] += _sharesAdded;
   ...
   772     userCollateralBalance[_borrower] += _collateralAmount;
   773     totalCollateral += _collateralAmount;
   ...
   813     userCollateralBalance[_borrower] -= _collateralAmount;
   ...
   815     totalCollateral -= _collateralAmount;
   ...
   863     _totalBorrow.amount -= _amountToRepay;
   864     _totalBorrow.shares -= _shares;
   ...
   867     userBorrowShares[_borrower] -= _shares;
   ...
   1008    totalAsset.amount -= _amountToAdjust;
   ...
   1011    _totalBorrow.amount -= _amountToAdjust;
   1012    _totalBorrow.shares -= _sharesToAdjust;

   • src/contracts/FraxlendPair.sol:252-253

   252     _totalAsset.amount -= uint128(_amountToTransfer);
   253     _totalAsset.shares -= _shares;

6. Using bools for storage incurs overhead (9 instances)

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

   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

   Deployment Gas Saved: 6855 Method Call Gas Saved: ≈26630

   • src/contracts/FraxlendWhitelist.sol:32,35,38

   32      mapping(address => bool) public oracleContractWhitelist;
   ...
   35      mapping(address => bool) public rateContractWhitelist;
   ...
   38      mapping(address => bool) public fraxlendDeployerWhitelist;

   • src/contracts/FraxlendPairCore.sol:78

   78      mapping(address => bool) public swappers;

   • src/contracts/FraxlendPairCore.sol:133-134,136-137

   133     bool public immutable borrowerWhitelistActive; // @note bool usage
   134     mapping(address => bool) public approvedBorrowers;
   ...
   136     bool public immutable lenderWhitelistActive; // @note bool usage
   137     mapping(address => bool) public approvedLenders;

   • src/contracts/FraxlendPairDeployer.sol:94

   94      mapping(address => bool) public deployedPairCustomStatusByAddress;

7. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied (11 instances)

   Deployment Gas Saved: 807 Method Call Gas Saved: ≈1101

   • src/contracts/libraries/SafeERC20.sol:22

   22      uint8 i = 0;

   • src/contracts/FraxlendWhitelist.sol:51,66,81

   51      for (uint256 i = 0; i < _addresses.length; i++) {
   ...
   66      for (uint256 i = 0; i < _addresses.length; i++) {
   ...
   81      for (uint256 i = 0; i < _addresses.length; i++) {

   • src/contracts/FraxlendPairCore.sol:265,270

   265     for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
   ...
   270     for (uint256 i = 0; i < _approvedLenders.length; ++i) {

   • src/contracts/FraxlendPair.sol:289,308

   289     for (uint256 i = 0; i < _lenders.length; i++) {
   ...
   308     for (uint256 i = 0; i < _borrowers.length; i++) {

   • src/contracts/FraxlendPairDeployer.sol:127,152,402

   127     for (i = 0; i < _lengthOfArray; ) {
   ...
   152     for (i = 0; i < _lengthOfArray; ) {
   ...
   402     for (uint256 i = 0; i < _lengthOfArray; ) {