Fraxlend (Frax Finance) contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L966-L1023

Vulnerability details

Impact

In src/contracts/FraxlendPairCore.sol liquidateClean(), if _sharesToLiquidate provided is too large by mistake, the following impacts can happen:

1. the liquidator may lose fund Since _collateralForLiquidator is upper bounded by _userCollateralBalance and the _repayAsset() function just uses _sharesToLiquidate.

2. liquidator's tradeoff The fee for liquidator could be cleanLiquidationFee or dirtyLiquidationFee, depending on the _leftoverCollateral. On the edge cases, the liquidator may get dirtyLiquidationFee if the transaction is not timely mined by a small difference of a few wei. But if the liquidator provides a larger _sharesToLiquidate to make sure the cleanLiquidationFee is get, the profit would be less. According to the current pattern, the choice would be a tradeoff.

3. In accurate totalAsset accounting the totalAsset accounting, it does not track the actual balance of the pair contract, just debit and credit the amount and shares provided. If the wrong share amount is transferred to the vault, the bookkeeping record will be inaccurate.

However, the above impacts can all be mitigated, unexpected loss, liquidator's tradeoff and potential inaccuracy can be avoided if the actual _sharesToLiquidate is recalculated by _collateralForLiquidator, in this way the liquidation process can be more robust to user errors.

Proof of Concept

The collateral can be received by liquidator _collateralForLiquidator is calculated from _sharesToLiquidate -> _liquidationAmountInCollateralUnits -> _optimisticCollateralForLiquidator -> _leftoverCollateral -> _collateralForLiquidator It has a upper bound of _userCollateralBalance

966-991:
        uint256 _userCollateralBalance = userCollateralBalance[_borrower];
        uint128 _borrowerShares = userBorrowShares[_borrower].toUint128();

        int256 _leftoverCollateral;
        {
            uint256 _liquidationAmountInCollateralUnits = ((_totalBorrow.toAmount(_sharesToLiquidate, false) *
                _exchangeRate) / EXCHANGE_PRECISION);

            uint256 _optimisticCollateralForLiquidator = (_liquidationAmountInCollateralUnits *
                (LIQ_PRECISION + cleanLiquidationFee)) / LIQ_PRECISION;

            _leftoverCollateral = (_userCollateralBalance.toInt256() - _optimisticCollateralForLiquidator.toInt256());

            _collateralForLiquidator = _leftoverCollateral <= 0
                ? _userCollateralBalance
                : (_liquidationAmountInCollateralUnits * (LIQ_PRECISION + dirtyLiquidationFee)) / LIQ_PRECISION;
        }

The amount to repay the debt by liquidator is calculated from _sharesToLiquidate:

993:        uint128 _amountLiquidatorToRepay = (_totalBorrow.toAmount(_sharesToLiquidate, true)).toUint128();

1019:       _repayAsset(_totalBorrow, _amountLiquidatorToRepay, _sharesToLiquidate, msg.sender, _borrower); // liquidator repays shares on behalf of borrower

And the collateral received is given by:

        _removeCollateral(_collateralForLiquidator, msg.sender, _borrower);

Imagine, _sharesToLiquidate provided is too large by mistake, but according to the current rule, _collateralForLiquidator has a maximum of _userCollateralBalance. If the liquidator intended to set _sharesToLiquidate to 100, but mistakenly set it to 1000, the _repayAsset() will still transfer 1000 from the liquidator, in this case, the liquidator will lose fund of 900.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Recalculate _amountLiquidatorToRepay from _collateralForLiquidator.

        uint256 _fee = _leftoverCollateral <= 0
                ? cleanLiquidationFee : dirtyLiquidationFee;

        uint128 _sharesAdjust = _totalBorrow.toShares(_collateralForLiquidator * LIQ_PRECISION / (LIQ_PRECISION + _fee) * EXCHANGE_PRECISION / _exchangeRate, true);

        uint128 _amountLiquidatorToRepay = (_totalBorrow.toAmount(_sharesAdjust, true)).toUint128();

In this way, the potential unexpected results can be mitigated:

1. the extra repay shares would not be transferred.
2. liquidators can provide a slightly larger repay share to guarantee the cleanLiquidationFee.
3. bookkeeping would be more accurate and robust.

MISSING CHECKS FOR ADDRESS(0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES

src/contracts/FraxlendPairCore.sol

172-175:
        CIRCUIT_BREAKER_ADDRESS = _circuitBreaker;
        COMPTROLLER_ADDRESS = _comptrollerAddress;
        TIME_LOCK_ADDRESS = _timeLockAddress;
        FRAXLEND_WHITELIST_ADDRESS = _fraxlendWhitelistAddress;

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

These addresses are immutable, there is no way to change them if mistakenly set to 0 address. In which case may lead to redeployment of contracts.

EVENT IS MISSING INDEXED FIELDS

Each event should use three indexed fields if there are three or more fields.

src/contracts/FraxlendPair.sol
228:    event WithdrawFees(uint128 _shares, address _recipient, uint256 _amountToTransfer);

src/contracts/FraxlendPairCore.sol
376-382:
        event AddInterest(
            uint256 _interestEarned,
            uint256 _rate,
            uint256 _deltaTime,
            uint256 _feesAmount,
            uint256 _feesShare
        );

389:    event UpdateRate(uint256 _ratePerSec, uint256 _deltaTime, uint256 _utilizationRate, uint256 _newRatePerSec);

696-701:
        event BorrowAsset(
            address indexed _borrower,
            address indexed _receiver,
            uint256 _borrowAmount,
            uint256 _sharesAdded
        );

760:    event AddCollateral(address indexed _sender, address indexed _borrower, uint256 _collateralAmount);

846:    event RepayAsset(address indexed _sender, address indexed _borrower, uint256 _amountToRepay, uint256 _shares);

897-904:
        event Liquidate(
            address indexed _borrower,
            uint256 _collateralForLiquidator,
            uint256 _sharesToLiquidate,
            uint256 _amountLiquidatorToRepay,
            uint256 _sharesToAdjust,
            uint256 _amountToAdjust
        );

1045-1052:
        event LeveragedPosition(
            address indexed _borrower,
            address _swapperAddress,
            uint256 _borrowAmount,
            uint256 _borrowShares,
            uint256 _initialCollateralAmount,
            uint256 _amountCollateralOut
        );

1143-1149:
        event RepayAssetWithCollateral(
            address indexed _borrower,
            address _swapperAddress,
            uint256 _collateralToSwap,
            uint256 _amountAssetOut,
            uint256 _sharesRepaid
        );

Residual debt shares after liquidation

There would be residual debt shares in the borrower balance after liquidation. Since repaid _shares in liquidation is at discount, userBorrowShares[_borrower] will not be 0 after liquidation.

src/contracts/FraxlendPairCore.sol

929-932:
        _collateralForLiquidator =
            (((_totalBorrow.toAmount(_shares, false) * _exchangeRate) / EXCHANGE_PRECISION) *
                (LIQ_PRECISION + cleanLiquidationFee)) /
            LIQ_PRECISION;

It can be seen that _shares is discounted by:

        (LIQ_PRECISION + cleanLiquidationFee)) / LIQ_PRECISION

Suggestion: write off the residual debt shares in userBorrowShares[_borrower] after liquidation.

FOR-LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.

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

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

src/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++) {

Suggestion: Cache the length before the loop.

    uint length = names.length;

FOR-LOOP unchecked{++i} COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

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

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

src/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++) {

src/contracts/FraxlendPairDeployer.sol
130:                i++;
158:                i++;

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

Suggestion: For readability, uncheck ++i, just like in src/contracts/FraxlendPairDeployer.sol

407-409:
            unchecked {
                i = i + 1;
            }

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

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.

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

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

src/contracts/FraxlendPairDeployer.sol
402:        for (uint256 i = 0; i < _lengthOfArray; ) {

src/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++) {

The demo of the loop gas comparison can be seen here.

SPLITTING REQUIRE() STATEMENTS THAT USE &&

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.

The demo of the gas comparison can be seen here.

src/contracts/LinearInterestRate.sol

57-68:
        require(
            _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
            "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
        );
        require(
            _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
            "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
        );
        require(
            _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
            "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
        );

REDUCE THE SIZE OF ERROR MESSAGES

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.

src/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");

src/contracts/LinearInterestRate.sol
57-68:
        require(
            _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
            "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
        );
        require(
            _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
            "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
        );
        require(
            _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
            "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
        );

Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) (reference)

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. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

The demo of the gas comparison can be seen here.

The following files have multiple require() statements can use custom errors:

src/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");

src/contracts/LinearInterestRate.sol
57-68:
        require(
            _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT,
            "LinearInterestRate: _minInterest < MAX_INT && _minInterest <= _vertexInterest && _minInterest >= MIN_INT"
        );
        require(
            _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT,
            "LinearInterestRate: _maxInterest <= MAX_INT && _vertexInterest <= _maxInterest && _maxInterest > MIN_INT"
        );
        require(
            _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0,
            "LinearInterestRate: _vertexUtilization < MAX_VERTEX_UTIL && _vertexUtilization > 0"
        );

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

src/contracts/FraxlendPairCore.sol

475:        _totalBorrow.amount += uint128(_interestEarned);
476:        _totalAsset.amount += uint128(_interestEarned);
484:        _totalAsset.shares += uint128(_feesShare);
566:        _totalAsset.amount += _amount;
567:        _totalAsset.shares += _shares;
718:        _totalBorrow.amount += _borrowAmount;
719:        _totalBorrow.shares += uint128(_sharesAdded);
724:        userBorrowShares[msg.sender] += _sharesAdded;
772:        userCollateralBalance[_borrower] += _collateralAmount;
773:        totalCollateral += _collateralAmount;

643:        _totalAsset.amount -= _amountToReturn;
644:        _totalAsset.shares -= _shares;
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;

Use bytes32 instead of string

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32.

Bytes32 can be used instead of string in the following:

src/contracts/FraxlendPairCore.sol

51:     string public version = "1.0.0";
92:     string internal nameOfContract;

Non changeable variables can be set to constant or immutable

src/contracts/FraxlendPairCore.sol

51:     string public version = "1.0.0";
92:     string internal nameOfContract;

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

src/contracts/FraxlendPairCore.sol

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

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If a function modifier such as onlyOwner, approvedLender() or approvedBorrower 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.

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.

src/contracts/FraxlendPair.sol
204:    function setTimeLock(address _newAddress) external onlyOwner {
234:    function withdrawFees(uint128 _shares, address _recipient) external onlyOwner returns (uint256 _amountToTransfer) {
274:    function setSwapper(address _swapper, bool _approval) external onlyOwner {
288:    function setApprovedLenders(address[] calldata _lenders, bool _approval) external approvedLender(msg.sender) {

src/contracts/FraxlendPairDeployer.sol
170:    function setCreationCode(bytes calldata _creationCode) external onlyOwner {

src/contracts/FraxlendWhitelist.sol
50:     function setOracleContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
65:     function setRateContractWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {
80:     function setFraxlendDeployerWhitelist(address[] calldata _addresses, bool _bool) external onlyOwner {


src/contracts/FraxlendPairCore.sol
583-589:
    function deposit(uint256 _amount, address _receiver)
        external
        nonReentrant
        isNotPastMaturity
        whenNotPaused
        approvedLender(_receiver)
        returns (uint256 _sharesReceived)

602-608:
    function mint(uint256 _shares, address _receiver)
        external
        nonReentrant
        isNotPastMaturity
        whenNotPaused
        approvedLender(_receiver)
        returns (uint256 _amountReceived)

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

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

739-750:
    function borrowAsset(
        uint256 _borrowAmount,
        uint256 _collateralAmount,
        address _receiver
    )
        external
        isNotPastMaturity
        whenNotPaused
        nonReentrant
        isSolvent(msg.sender)
        approvedBorrower
        returns (uint256 _shares)
        
1062-1075:
    function leveragedPosition(
        address _swapperAddress,
        uint256 _borrowAmount,
        uint256 _initialCollateralAmount,
        uint256 _amountCollateralOutMin,
        address[] memory _path
    )
        external
        isNotPastMaturity
        nonReentrant
        whenNotPaused
        approvedBorrower
        isSolvent(msg.sender)
        returns (uint256 _totalCollateralBalance)