Fraxlend (Frax Finance) contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L409-L497

Vulnerability details

Impact

For a given pair under the custom deployment, a maturity date can be set and can be past. It is possible that the _addInterest function is not called just before the maturity date. This means that the interest accruing over the period before the maturity date is not calculated or recorded yet. When the _addInterest function is called after the maturity date, penaltyRate is applied for both the periods before and after the maturity date. Retroactively applying penaltyRate to the period before the maturity is not fair to the borrowers because that period should be associated with the normal interest rate and the penalty rate can be much higher as it is not capped like the normal interest rate.

Proof of Concept

The following steps can occur.

1. For a deployed pair, a borrower borrows some asset tokens after providing some collateral tokens.
2. One day before the maturity date, the _addInterest function is called. currentRateInfo.lastTimestamp is recorded accordingly.
3. Afterwards, _addInterest is not called anymore after Step 2's time and just before the maturity date.
4. One day after the maturity date, _addInterest is called. Because of the following logic in the _addInterest function below, penaltyRate is assigned to _newRate, which is applied over block.timestamp - _currentRateInfo.lastTimestamp to calculate _interestEarned. _interestEarned eventually is added to totalAsset.amount and totalBorrow.amount. Because penaltyRate is not capped and can be much higher than the normal interest rate, the borrow amount is higher than it should be. In other words, the borrow amount can be smaller if the duration of the day before the maturity date is applied fairly with the normal interest rate instead of the penalty rate.

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L409-L497

    function _addInterest()
        internal
        returns (
            uint256 _interestEarned,
            uint256 _feesAmount,
            uint256 _feesShare,
            uint64 _newRate
        )
    {
        // Add interest only once per block
        CurrentRateInfo memory _currentRateInfo = currentRateInfo;
		
        ...
		
        // Pull some data from storage to save gas
        VaultAccount memory _totalAsset = totalAsset;
        VaultAccount memory _totalBorrow = totalBorrow;
		
        if (_totalBorrow.shares == 0 || paused()) {
		
            ...
			
        } else {
            // We know totalBorrow.shares > 0
            uint256 _deltaTime = block.timestamp - _currentRateInfo.lastTimestamp;
			
            ...
			
            if (_isPastMaturity()) {
                _newRate = uint64(penaltyRate);
            } else {
                ...
            }
			
            ...
			
            _currentRateInfo.ratePerSec = _newRate;
            _currentRateInfo.lastTimestamp = uint64(block.timestamp);
            _currentRateInfo.lastBlock = uint64(block.number);

            // Calculate interest accrued
            _interestEarned = (_deltaTime * _totalBorrow.amount * _currentRateInfo.ratePerSec) / 1e18;

            // Accumulate interest and fees, only if no overflow upon casting
            if (
                _interestEarned + _totalBorrow.amount <= type(uint128).max &&
                _interestEarned + _totalAsset.amount <= type(uint128).max
            ) {
                _totalBorrow.amount += uint128(_interestEarned);
                _totalAsset.amount += uint128(_interestEarned);
				
                ...
            }

            // Effects: write to storage
            totalAsset = _totalAsset;
            currentRateInfo = _currentRateInfo;
            totalBorrow = _totalBorrow;
        }
    }

Tools Used

VSCode

Recommended Mitigation Steps

When calling the _addInterest function, if the current block.timestamp is past maturity date, the normal interest rate should be used first to calculate the interest over the period just before the maturity date, and then the penalty rate should be used to calculate the interest over the period after the maturity date.

[L-01] MISSING RETURN VALUE CHECK FOR APPROVE()

Some tokens do not revert but return false when approval fails. Please consider checking the return values of the following approve functions to prevent silent approval failures.

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

[L-02] MISSING ZERO-ADDRESS CHECK FOR CRITICAL ADDRESSES

To prevent unintended behaviors, the critical address inputs should be checked against address(0). Please consider checking the following address variables.

contracts\FraxlendPairCore.sol
	163-168:
		(
			address _circuitBreaker,
			address _comptrollerAddress,
			address _timeLockAddress,
			address _fraxlendWhitelistAddress
		) = abi.decode(_immutables, (address, address, address, address));

	179-187:
		(
			address _asset,
			address _collateral,
			address _oracleMultiply,
			address _oracleDivide,
			uint256 _oracleNormalization,
			address _rateContract,

		) = abi.decode(_configData, (address, address, address, address, uint256, address, bytes));

contracts\FraxlendPairDeployer.sol
	98-103:
                constructor(
                        address _circuitBreaker,
                        address _comptroller,
                        address _timelock,
                        address _fraxlendWhitelist
                ) Ownable() {

[N-01] STATE VARIABLE WITH CHANGEABLE VALUES CAN BE NAMED WITH LOWERCASE LETTERS

It is a convention to use lowercase letters for naming state variables that have changeable values. Please consider renaming the following variable by using lowercase letters because its value gets assigned in another function.

contracts\FraxlendPairCore.sol
    address public TIME_LOCK_ADDRESS;

[N-02] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers used in the following code with constants.

contracts\FraxlendPair.sol
  105: _assetsPerUnitShare = totalAsset.toAmount(1e18, false);

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

contracts\FraxlendPairDeployer.sol
  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);

contracts\VariableInterestRate.sol
  69: uint256 _deltaUtilization = ((MIN_UTIL - _utilization) * 1e18) / MIN_UTIL;
  76: uint256 _deltaUtilization = ((_utilization - MAX_UTIL) * 1e18) / (UTIL_PREC - MAX_UTIL);

[N-03] UNDERSCORES IN NUMBER LITERALS CAN BE USED

Underscores in number literals can be used for improving readability and maintainability. Please consider using underscores for the following number literals.

contracts\FraxlendPairDeployer.sol
  49: uint256 public DEFAULT_MAX_LTV = 75000; // 75% with 1e5 precision
  51: uint256 public DEFAULT_LIQ_FEE = 10000; // 10% with 1e5 precision

contracts\LinearInterestRate.sol
  34: uint256 private constant MAX_INT = 146248508681; // 10,000% annual rate

contracts\VariableInterestRate.sol
  40: uint64 private constant MIN_INT = 79123523; // 0.25% annual rate
  41: uint64 private constant MAX_INT = 146248508681; // 10,000% annual rate 

[N-04] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following constructor and functions. Please consider completing NatSpec comments for them.

contracts\FraxlendPairCore.sol
  151: constructor(   
  393: function addInterest()
  409: function _addInterest()

contracts\FraxlendPairDeployer.sol
  191: function _deployFirst( 

[N-05] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

contracts\FraxlendPair.sol
  2: pragma solidity ^0.8.15;

contracts\FraxlendPairConstants.sol
  2: pragma solidity ^0.8.15;

contracts\FraxlendPairCore.sol
  2: pragma solidity ^0.8.15;

contracts\FraxlendPairDeployer.sol
  2: pragma solidity ^0.8.15;

contracts\FraxlendPairHelper.sol
  2: pragma solidity ^0.8.15;

contracts\FraxlendWhitelist.sol
  2: pragma solidity ^0.8.15;

contracts\LinearInterestRate.sol
  2: pragma solidity ^0.8.15;

contracts\VariableInterestRate.sol
  2: pragma solidity ^0.8.15;

contracts\interfaces\IERC4626.sol
  2: pragma solidity >=0.8.15;

contracts\interfaces\IFraxlendPair.sol
  2: pragma solidity >=0.8.15;

contracts\interfaces\IFraxlendWhitelist.sol
  2: pragma solidity >=0.8.15;

contracts\interfaces\IRateCalculator.sol
  2: pragma solidity >=0.8.15;

contracts\interfaces\ISwapper.sol
  2: pragma solidity >=0.8.15;

contracts\libraries\SafeERC20.sol
  2: pragma solidity ^0.8.15;

contracts\libraries\VaultAccount.sol
  2: pragma solidity ^0.8.15;

[G-01] block.number - _currentRateInfo.lastBlock OF _rateData IS NOT USED IN getNewRate FUNCTION AND CAN BE REMOVED

block.number - \_currentRateInfo.lastBlock of _rateData in the following code is never used in the getNewRate functions of the LinearInterestRate and VariableInterestRate contract. Hence, it can be removed from _rateData, and the getNewRate functions can be adjusted accordingly to save deployment and run-time gas.

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L450-L456

	bytes memory _rateData = abi.encode(
		_currentRateInfo.ratePerSec,
		_deltaTime,
		_utilizationRate,
		block.number - _currentRateInfo.lastBlock
	);
	_newRate = IRateCalculator(rateContract).getNewRate(_rateData, rateInitCallData);

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/LinearInterestRate.sol#L76-L92

    function getNewRate(bytes calldata _data, bytes calldata _initData) external pure returns (uint64 _newRatePerSec) {
        requireValidInitData(_initData);
        (, , uint256 _utilization, ) = abi.decode(_data, (uint64, uint256, uint256, uint256));
        (uint256 _minInterest, uint256 _vertexInterest, uint256 _maxInterest, uint256 _vertexUtilization) = abi.decode(
            _initData,
            (uint256, uint256, uint256, uint256)
        );
        if (_utilization < _vertexUtilization) {
            uint256 _slope = ((_vertexInterest - _minInterest) * UTIL_PREC) / _vertexUtilization;
            _newRatePerSec = uint64(_minInterest + ((_utilization * _slope) / UTIL_PREC));
        } else if (_utilization > _vertexUtilization) {
            uint256 _slope = (((_maxInterest - _vertexInterest) * UTIL_PREC) / (UTIL_PREC - _vertexUtilization));
            _newRatePerSec = uint64(_vertexInterest + (((_utilization - _vertexUtilization) * _slope) / UTIL_PREC));
        } else {
            _newRatePerSec = uint64(_vertexInterest);
        }
    }

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

    function getNewRate(bytes calldata _data, bytes calldata _initData) external pure returns (uint64 _newRatePerSec) {
        (uint64 _currentRatePerSec, uint256 _deltaTime, uint256 _utilization, ) = abi.decode(
            _data,
            (uint64, uint256, uint256, uint256)
        );
        if (_utilization < MIN_UTIL) {
            uint256 _deltaUtilization = ((MIN_UTIL - _utilization) * 1e18) / MIN_UTIL;
            uint256 _decayGrowth = INT_HALF_LIFE + (_deltaUtilization * _deltaUtilization * _deltaTime);
            _newRatePerSec = uint64((_currentRatePerSec * INT_HALF_LIFE) / _decayGrowth);
            if (_newRatePerSec < MIN_INT) {
                _newRatePerSec = MIN_INT;
            }
        } else if (_utilization > MAX_UTIL) {
            uint256 _deltaUtilization = ((_utilization - MAX_UTIL) * 1e18) / (UTIL_PREC - MAX_UTIL);
            uint256 _decayGrowth = INT_HALF_LIFE + (_deltaUtilization * _deltaUtilization * _deltaTime);
            _newRatePerSec = uint64((_currentRatePerSec * _decayGrowth) / INT_HALF_LIFE);
            if (_newRatePerSec > MAX_INT) {
                _newRatePerSec = MAX_INT;
            }
        } else {
            _newRatePerSec = _currentRatePerSec;
        }
    }

[G-02] REDUNDANT TYPE CONVERSION CAN BE AVOIDED

Converting a variable type when not needed costs more unnecessary gas. 1e36 does not need to be explicitly casted for the following code.

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

uint256 _price = uint256(1e36);

[G-03] STATE VARIABLES WITH VALUES THAT DO NOT CHANGE CAN BE DECLARED AS CONSTANTS

Reading constants save more gas than reading state variables. Please consider declaring the following state variables as constants since their values do not change.

contracts\FraxlendPairDeployer.sol
  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

[G-04] STATE VARIABLE WHOSE VALUE IS ONLY ASSIGNED IN CONSTRUCTOR CAN BE IMMUTABLE

Providing an immutable keyword to the state variable whose value is only assigned in the constructor can help save gas. Please consider declaring the following state variables as immutables.

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

[G-05] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

contracts\FraxlendPair.sol
  289: for (uint256 i = 0; i < _lenders.length; i++) {
  308: for (uint256 i = 0; i < _borrowers.length; i++) {

contracts\FraxlendPairCore.sol
  265: for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
  270: for (uint256 i = 0; i < _approvedLenders.length; ++i) {

contracts\FraxlendPairDeployer.sol
  127: for (i = 0; i < _lengthOfArray; ) {
  152: for (i = 0; i < _lengthOfArray; ) {
  402: for (uint256 i = 0; i < _lengthOfArray; ) {

contracts\FraxlendWhitelist.sol
  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++) {

contracts\libraries\SafeERC20.sol
  27: for (i = 0; i < 32 && data[i] != 0; i++) {

[G-06] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, _lenders.length in the following code can be cached outside of the loop like uint256 _lendersLength = _lenders.length, and i < _lendersLength can be used for each iteration.

contracts\FraxlendPair.sol
  289: for (uint256 i = 0; i < _lenders.length; i++) {
  308: for (uint256 i = 0; i < _borrowers.length; i++) {

contracts\FraxlendPairCore.sol
  265: for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
  270: for (uint256 i = 0; i < _approvedLenders.length; ++i) {

contracts\FraxlendWhitelist.sol
  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++) {

[G-07] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

contracts\FraxlendPair.sol
  289: for (uint256 i = 0; i < _lenders.length; i++) {
  308: for (uint256 i = 0; i < _borrowers.length; i++) {

contracts\FraxlendWhitelist.sol
  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++) {

[G-08] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loops, if increasing the counter variable is very unlikely to overflow, then unchecked {++i} at the end of the loop block can be used, where i is the counter variable.

contracts\FraxlendPair.sol
  289: for (uint256 i = 0; i < _lenders.length; i++) {
  308: for (uint256 i = 0; i < _borrowers.length; i++) {

contracts\FraxlendPairCore.sol
  265: for (uint256 i = 0; i < _approvedBorrowers.length; ++i) {
  270: for (uint256 i = 0; i < _approvedLenders.length; ++i) {

contracts\FraxlendWhitelist.sol
  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++) {

contracts\libraries\SafeERC20.sol
  27: for (i = 0; i < 32 && data[i] != 0; i++) {

[G-09] REVERT REASON STRINGS CAN BE SHORTENED TO FIT IN 32 BYTES

Revert reason strings that are longer than 32 bytes need more memory space and more mstore operation(s) than these that are shorter than 32 bytes when reverting. Please consider shortening the following revert reason strings.

contracts\FraxlendPairDeployer.sol
  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");  

contracts\LinearInterestRate.sol
  59: "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
  63: "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
  67: "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"

[G-10] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REQUIRE() WITH REASON STRING

revert with custom error can cost less gas than require() with reason string. Please consider using revert with custom error to replace the following require().

contracts\FraxlendPairDeployer.sol
  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(
  399: require(msg.sender == CIRCUIT_BREAKER_ADDRESS, "Circuit Breaker only");

contracts\LinearInterestRate.sol
  57: require(
  61: require(
  65: require(