Fraxlend (Frax Finance) contest - ElKu's results

Low Risk Issues

Summary Of Findings:

Details on Findings:

1. <ins>Zero-check is not performed for address</ins>

In the FraxlendPair.sol, address is not checked for a zero value before assigning it to the TIME_LOCK_ADDRESS in the setTimeLock function.

Mitigation would be to add a require statement in the function as shown below:

    function setTimeLock(address _newAddress) external onlyOwner {
        emit SetTimeLock(TIME_LOCK_ADDRESS, _newAddress);
        require(_newAddress != address(0), "Zero Address");
        TIME_LOCK_ADDRESS = _newAddress;  // @audit zero check
    }

Similarly in the FraxlendPairDeployer.sol, the constructor doesn't perform any zero checks for important addresses.

    constructor(  
        address _circuitBreaker,
        address _comptroller,
        address _timelock,
        address _fraxlendWhitelist
    ) Ownable() {
        CIRCUIT_BREAKER_ADDRESS = _circuitBreaker;
        COMPTROLLER_ADDRESS = _comptroller;
        TIME_LOCK_ADDRESS = _timelock;
        FRAXLEND_WHITELIST_ADDRESS = _fraxlendWhitelist;
    }

Again, in the FraxlendPairCore.sol file, the constructor doesn't check for zero addresses before assigning important contract addresses.

Line 162: {
            (
                address _circuitBreaker,
                address _comptrollerAddress,
                address _timeLockAddress,
                address _fraxlendWhitelistAddress
            ) = abi.decode(_immutables, (address, address, address, address));

            // Deployer contract
            DEPLOYER_ADDRESS = msg.sender;
            CIRCUIT_BREAKER_ADDRESS = _circuitBreaker;
            COMPTROLLER_ADDRESS = _comptrollerAddress;
            TIME_LOCK_ADDRESS = _timeLockAddress;
            FRAXLEND_WHITELIST_ADDRESS = _fraxlendWhitelistAddress;
        }

Line 190:   assetContract = IERC20(_asset);
Line 191:   collateralContract = IERC20(_collateral);

2. <ins>Use <= instead of < when checking if Vault has enough funds to transfer</ins>

Though it is unlikely that this will cause a problem, for the readability of logic, it is better to use <=. If the fund in the vault is exactly equal to the fund requested then the function shouldn't revert.

There are 3 instances of this:

1. _redeem function
2. _borrowAsset function
3. repayAssetWithCollateral function

Line  638:    if (_assetsAvailable < _amountToReturn) {
Line  712:    if (_assetsAvailable < _borrowAmount) {
Line 1200:    if (_amountAssetOut < _amountAssetOutMin) {

Non-Critical Issues

Summary Of Findings:

Details on Findings:

1. <ins>Non-library/interface files should use fixed compiler versions, not floating ones</ins>

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://swcregistry.io/docs/SWC-103

All the files under scope uses floating pragma like the one below:

pragma solidity ^0.8.15;

Mitigation would be to change it to this:

pragma solidity 0.8.15;

2. <ins>Wrong Natspec comments</ins>

See the following instances:

1. line 284

@notice The ```setApprovedLenders``` function sets a given set of addresses to the whitelist

while the function can add the addresses to the blacklist as well (and not only whitelist)

2. line 303

@notice The ```setApprovedBorrowers``` function sets a given array of addresses to the whitelist

As the function can add the addresses to the blacklist as well (and not only whitelist)

3. <ins>Misleading function names</ins>

In the FraxlendWhitelist file, the 3 functions, setOracleContractWhitelist, setRateContractWhitelist and setFraxlendDeployerWhitelist are called whitelist but they can be also used to blacklist an address.

4. <ins>Not emitting any event for unnatural exceptions</ins>

In the FraxlendPairCore.sol file, the if statement here should have an else which should emit an event. As overflow in the totalBorrow.amount or totalAsset.amount is a situation worth getting notified.

            if (
                _interestEarned + _totalBorrow.amount <= type(uint128).max &&
                _interestEarned + _totalAsset.amount <= type(uint128).max
            )

Gas Optimizations

Summary Of Findings:

Detailed Report on Gas Optimization Findings:

1. <ins>Use unchecked blocks whenever possible</ins>

Starting solidity 0.8.0, you have overflow and underflow protection without using safeMath library. But these checks cost extra gas. And in some cases when we are sure that the result wont overflow or underflow, we can avoid such checks by putting the calculations inside an unchecked block.

The following instances shows where this could be applied:

1. File: FraxlendPairCore.sol

//_addInterest function
Line 441:       uint256 _deltaTime = block.timestamp - _currentRateInfo.lastTimestamp;

Line 446:       uint256 _utilizationRate = (UTIL_PREC * _totalBorrow.amount) / _totalAsset.amount;

Line 468:       _interestEarned = (_deltaTime * _totalBorrow.amount * _currentRateInfo.ratePerSec) / 1e18;

Line 472-484:   _interestEarned + _totalBorrow.amount <= type(uint128).max &&
                _interestEarned + _totalAsset.amount <= type(uint128).max
              ) {
                _totalBorrow.amount += uint128(_interestEarned);
                _totalAsset.amount += uint128(_interestEarned);
                if (_currentRateInfo.feeToProtocolRate > 0) {
                    _feesAmount = (_interestEarned * _currentRateInfo.feeToProtocolRate) / FEE_PRECISION;

                    _feesShare = (_feesAmount * _totalAsset.shares) / (_totalAsset.amount - _feesAmount);

                    // Effects: Give new shares to this contract, effectively diluting lenders an amount equal to the fees
                    // We can safely cast because _feesShare < _feesAmount < interestEarned which is always less than uint128
                    _totalAsset.shares += uint128(_feesShare);

//_updateExchangeRate function
Line 536:       _price = _price / uint256(_answer);
Line 539:       _exchangeRate = _price / oracleNormalization;

//_redeem function
Line 643-644:   _totalAsset.amount -= _amountToReturn;
                _totalAsset.shares -= _shares;

//_borrowAsset function
Line 724:       userBorrowShares[msg.sender] += _sharesAdded;

//liquidateClean function
Line 1004-1012:  if (_leftoverBorrowShares > 0) {
                    // Write off bad debt
                    _sharesToAdjust = _leftoverBorrowShares;
                    _amountToAdjust = (_totalBorrow.toAmount(_sharesToAdjust, false)).toUint128();

                    // Effects: write to state
                    totalAsset.amount -= _amountToAdjust;

                    // Note: Ensure this memory stuct will be passed to _repayAsset for write to state
                    _totalBorrow.amount -= _amountToAdjust;
                    _totalBorrow.shares -= _sharesToAdjust;
                }

2. File: FraxlendPair.sol

//withdrawFees function
Line 252-253:  _totalAsset.amount -= uint128(_amountToTransfer);
               _totalAsset.shares -= _shares;

3. File: VaultAccount.sol

//toShares function
Line 26:       shares = shares + 1;

//toAmount function
Line 43:       amount = amount + 1;

After the above mentioned calculations are put inside an unchecked block, the gas savings as per foundry were as follows:

<ins>Foundry Gas report:</ins>

Summing up the total average gas saved in all the functions, we get 71,529.

2. <ins>Rewriting the returnDataToString function in a gas efficient way</ins>

In SafeERC20.sol, the original returnDataToString function can be rewritten as follows to make it more gas efficient:

    function returnDataToString(bytes memory data) internal pure returns (string memory) {
        if (data.length >= 64) {
            return abi.decode(data, (string));
        } else if (data.length == 32) {
            uint256 i = 0;   // saves casting operations from 256 to 8 bit
            while (i < 32 && data[i] != 0) {
                unchecked   //overflow safe.
                { ++i;
                }
            }
            bytes memory bytesArray = new bytes(i);
            //avoid redundant initilization to zero
            //limit of j is last value of i. calculated above. 
            //unchecked block for ++j
            for (uint256 j; j < i; ) {   
                bytesArray[i] = data[i];
                unchecked {
                    ++j;
                }
            }
            return string(bytesArray);
        } else {
            return "???";
        }
    }

The above single change reduced the gas usage in the FraxlendPairDeployer contract as given below:

<ins>Foundry Gas report:</ins>

Summing up the total average gas saved in all the three functions, we get 57,609.

3. <ins>Optimizing the for loops</ins>

The for loops in the code base generally follow the following pattern:

      for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
            approvedBorrowers[_approvedBorrowers[i]] = true;
        }

This can be optimized in 3 ways:

      len = _approvedBorrowers.length;  // 1. cache the array length
      for (uint256 i; i < len; ) {  // 2. remove redundant initialization of i
            approvedBorrowers[_approvedBorrowers[i]] = true;
            unchecked {  // 3. using unchecked block for incrementing the index
              ++i;
            }
        }

Note that the above optimization is already seen used in the FraxlendPairDeployer.sol file.

The following instances could be optimized in this way:

1. FraxlendPair.sol

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

2. FraxlendPairCore.sol

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

3. FraxlendWhitelist.sol

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

After the above for loops were optimized, the gas savings as per foundry were as follows:

<ins>Foundry Gas report:</ins>

Summing up the total average gas saved in all the functions, we get 15,998.

Conclusions:

The above 3 major changes saved a very significant amount of gas in the overall code base. The exact amounts are tabulated below:

TOTAL GAS SAVED = 1,45,136