Incorrect implementation in fraxlend pair violating standard causes DOS

Summary

The incorrect implementation of previewWithdraw in FraxlendPair.sol will cause transactions to fail when there is insufficient liquidity, as users will be unable to properly simulate withdrawals since the function doesn't properly check available liquidity before calculating shares, violating the ERC4626 standard. Also, in maxWithdraw/maxRedeem users will be unable to determine their maximum withdrawable amount when all assets are borrowed, when the vault is not fully utilized, the output amount will be incorrect, violating the ERC4626 standard.

Root Cause

According to the ERC4626 standard, the preview withdraw and redeem.

FraxlendPair.sol, the previewRedeem and previewWithdraw functions simply converts assets to shares using total asset calculation without checking available liquidity, it expected to allows an on-chain or off-chain user to simulate the effects of their withdrawal at the current block, given current on-chain conditions so when an exterternal protocol or user tries to. get the onchain preview based on the current block, and redeem/withdraw at same time, the transaction will fail

  function previewWithdraw(uint256 _amount) external view returns (uint256 _sharesToBurn) {
        (,,,, VaultAccount memory _totalAsset,) = previewAddInterest();
        _sharesToBurn = _totalAsset.toShares(_amount, true);
    }

    function previewRedeem(uint256 _shares) external view returns (uint256 _assets) {
        (,,,, VaultAccount memory _totalAsset,) = previewAddInterest();
        _assets = _totalAsset.toAmount(_shares, false);
    }

Similarly in LendingAssetVault, the previewWithdraw and redeem simulates all the accrued interest in the whitelisted vaults without the considerations of available assets, so the expected returns will be assets + interest.

The standard expects that transaction MAY revert due to other conditions that would also cause withdraw to revert This causes previewRedeem and previewWithdraw to return correct shares/amount when all assets are borrowed, which violates the ERC4626 standard.
Simulation of actual withdrawal effects
Return value that would actually work in a real withdraw call
Must not revert due to insufficient liquidity And maxWithdraw/maxRedeem, According to the ERC4626 standard, the max withdraw and redeem are expected to factor in the following:
Maximum amount of the underlying asset that can be withdrawn from the owner balance in the Vault, through a withdraw call.
MUST return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

Also, in the case of maxWithdraw and maxRedeem functions incorrectly calculate available assets by subtracting borrowed amounts which is not dependent on the total assets and interest acrued.

    function maxWithdraw(address _owner) external view returns (uint256 _maxAssets) {
        if (isWithdrawPaused) return 0;
        (,, uint256 _feesShare,, VaultAccount memory _totalAsset, VaultAccount memory _totalBorrow) =
            previewAddInterest();
        // Get the owner balance and include the fees share if owner is this contract
        uint256 _ownerBalance = _owner == address(this) ? balanceOf(_owner) + _feesShare : balanceOf(_owner);

        // Return the lower of total assets in contract or total assets available to _owner
        uint256 _totalAssetsAvailable = _totalAssetAvailable(_totalAsset, _totalBorrow, true);
        uint256 _totalUserWithdraw = _totalAsset.toAmount(_ownerBalance, false);
        _maxAssets = _totalAssetsAvailable < _totalUserWithdraw ? _totalAssetsAvailable : _totalUserWithdraw;
    }

    function maxRedeem(address _owner) external view returns (uint256 _maxShares) {
        if (isWithdrawPaused) return 0;
        (,, uint256 _feesShare,, VaultAccount memory _totalAsset, VaultAccount memory _totalBorrow) =
            previewAddInterest();

        // Calculate the total shares available
        uint256 _totalAssetsAvailable = _totalAssetAvailable(_totalAsset, _totalBorrow, true);
        uint256 _totalSharesAvailable = _totalAsset.toShares(_totalAssetsAvailable, false);

        // Get the owner balance and include the fees share if owner is this contract
        uint256 _ownerBalance = _owner == address(this) ? balanceOf(_owner) + _feesShare : balanceOf(_owner);
        _maxShares = _totalSharesAvailable < _ownerBalance ? _totalSharesAvailable : _ownerBalance;
    }

The point when this is expected to return 0 is when the vault is paused

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0 This causes maxWithdraw and maxRedeem to return 0 when all assets are borrowed, and returns incorrect value when not all the allocation are been borrowed, which violates the ERC4626 standard.

So the issue is that: Current: Calculates based on available liquidity (subtracting borrowed amounts) Should be: Calculate theoretical maximum based on user's share ownership, regardless of current liquidity

Impact

The vault depositors cannot properly simulate withdrawals when there is insufficient liquidity, leading to:

DOS
Inability to properly integrate with other protocols expecting ERC4626 compliance
Violation of ERC4626 standard requirements
Inability to properly integrate with other protocols expecting ERC4626 compliance

PoC

Here is the foundry setup for the frax lending pair contract

Or Click to view

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

// forge
import {Test, console} from "forge-std/Test.sol";

// fraxlend
import {FraxlendPairDeployer, ConstructorParams} from "../src/contracts/FraxlendPairDeployer.sol";
import {FraxlendWhitelist} from "../src/contracts/FraxlendWhitelist.sol";
import {FraxlendPairRegistry} from "../src/contracts/FraxlendPairRegistry.sol";
import {FraxlendPair} from "../src/contracts/FraxlendPair.sol";
import {VariableInterestRate} from "../src/contracts/VariableInterestRate.sol";
import {IERC4626Extended} from "../src/contracts/interfaces/IERC4626Extended.sol";
import {ChainlinkSinglePriceOracle} from "../src/contracts/ChainlinkSinglePriceOracle.sol";
import "../src/contracts/mocks/LendingAssetVault.sol";

// mocks
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockERC20} from "../src/contracts/mocks/MockERC20.sol";

contract Fraxlend is Test {
    /*///////////////////////////////////////////////////////////////
                            GLOBAL VARIABLES
    ///////////////////////////////////////////////////////////////*/

    // external actors
    address internal user0 = vm.addr(uint256(keccak256("User0")));
    address internal user1 = vm.addr(uint256(keccak256("User1")));
    address internal user2 = vm.addr(uint256(keccak256("User2")));

    address[] internal users = [user0, user1, user2];
    uint256[] internal _fraxPercentages = [10000, 2500, 7500, 5000];

    // fraxlend protocol actors
    address internal comptroller = vm.addr(uint256(keccak256("comptroller")));
    address internal circuitBreaker = vm.addr(uint256(keccak256("circuitBreaker")));
    address internal timelock = vm.addr(uint256(keccak256("comptroller")));

    uint16 internal fee = 100;
    uint256 internal PRECISION = 10 ** 27;

    /*///////////////////////////////////////////////////////////////
                            TEST CONTRACTS
    ///////////////////////////////////////////////////////////////*/

    // fraxlend
    FraxlendPairDeployer internal _fraxDeployer;
    FraxlendWhitelist internal _fraxWhitelist;
    FraxlendPairRegistry internal _fraxRegistry;
    VariableInterestRate internal _variableInterestRate;

    FraxlendPair internal _fraxLPToken1Peas;

    MockERC20 internal _mockDai;
    MockERC20 internal _collateral;
    MockERC20 internal _tokenB;
    MockERC20 internal _tokenC;
    ChainlinkSinglePriceOracle internal _clOracle; 
    LendingAssetVault internal _lendingAssetVault;

    event Message(string message);

    /*///////////////////////////////////////////////////////////////
                            SETUP FUNCTIONS
    ///////////////////////////////////////////////////////////////*/

    function setUp() public {
        // deploy mock tokens
        _mockDai = new MockERC20("MockDai", "MDAI");
        _collateral = new MockERC20("Collateral", "COL");
        _tokenB = new MockERC20("TokenB", "TKB");
        _tokenC = new MockERC20("TokenC", "TKC");

        _mockDai.mint(address(this), 1000000 ether);
        _collateral.mint(address(this), 1000000 ether);
        _tokenB.mint(address(this), 1000000e6);
        _tokenC.mint(address(this), 1000000 ether);

        _deployVariableInterestRate();
        _deployFraxWhitelist();
        _deployFraxPairRegistry();
        _deployFraxPairDeployer();
        _deployFraxPairs();
        _setupActors();

        _deployLendingVault();


        vm.startPrank(address(2));
        _fraxLPToken1Peas.setExternalAssetVault(IERC4626Extended(address(_lendingAssetVault)));
        vm.stopPrank();
        
    }

    function _deployVariableInterestRate() internal {
        // These values taken from existing Fraxlend Variable Rate Contract
        _variableInterestRate = new VariableInterestRate(
            "[0.5 0.2@.875 5-10k] 2 days (.75-.85)",
            87500,
            200000000000000000,
            75000,
            85000,
            158247046,
            1582470460,
            3164940920000,
            172800
        );
    }

    function _deployFraxWhitelist() internal {
        _fraxWhitelist = new FraxlendWhitelist();
    }

    function _deployFraxPairRegistry() internal {
        address[] memory _initialDeployers = new address[](0);
        _fraxRegistry = new FraxlendPairRegistry(address(this), _initialDeployers);
    }

    function _deployFraxPairDeployer() internal {
        ConstructorParams memory _params =
            ConstructorParams(address(this), address(1), address(2), address(_fraxWhitelist), address(_fraxRegistry));
        _fraxDeployer = new FraxlendPairDeployer(_params);

        _fraxDeployer.setCreationCode(type(FraxlendPair).creationCode);

        address[] memory _whitelistDeployer = new address[](1);
        _whitelistDeployer[0] = address(this);

        _fraxWhitelist.setFraxlendDeployerWhitelist(_whitelistDeployer, true);

        address[] memory _registryDeployer = new address[](1);
        _registryDeployer[0] = address(_fraxDeployer);

        _fraxRegistry.setDeployers(_registryDeployer, true);
        emit Message("1");
    }

    function _deployFraxPairs() internal {
        // moving time to help out the twap
        vm.warp(block.timestamp + 1 days);

        _clOracle = new ChainlinkSinglePriceOracle(address(0));

        emit Message("1aa");

        _fraxLPToken1Peas = FraxlendPair(
            _fraxDeployer.deploy(
                abi.encode(
                    address(_mockDai), // asset
                    address(_collateral), // collateral
                    address(_clOracle), //oracle
                    5000, // 5000, // maxOracleDeviation
                    address(_variableInterestRate), //rateContract
                    1000, //fullUtilizationRate
                    75000, // maxLtv
                    10000, // uint256 _cleanLiquidationFee
                    9000, // uint256 _dirtyLiquidationFee
                    2000 //uint256 _protocolLiquidationFee
                )
            )
        );

        emit Message("1b");

        // deposit some asset
        IERC20(address(_mockDai)).approve(address(_fraxLPToken1Peas), type(uint256).max);
        _fraxLPToken1Peas.deposit(100000 ether, address(this));
    }

    function _deployLendingVault() internal {
        _lendingAssetVault = new LendingAssetVault("LDA", "FLDA", address(_mockDai));

        _lendingAssetVault.setVaultWhitelist(address(_fraxLPToken1Peas), true);
        address[] memory _vaults = new address[](1);
        _vaults[0] = address(_fraxLPToken1Peas);
        uint256[] memory _maxAllocations = new uint256[](1);
        _maxAllocations[0] = 100000 ether;
        _lendingAssetVault.setVaultMaxAllocation(_vaults, _maxAllocations);


        vm.startPrank(users[0]);
        IERC20(address(_mockDai)).approve(address(_lendingAssetVault), type(uint256).max);
        _lendingAssetVault.deposit(100000 ether, users[0]);
    }

    /*////////////////////////////////////////////////////////////////
                                    HELPERS
    ////////////////////////////////////////////////////////////////*/

    function _setupActors() internal {
        for (uint256 i; i < users.length; i++) {
            vm.deal(users[i], 1000000 ether);
            vm.startPrank(users[i]);
            // _weth.deposit{value: 1000000 ether}();

            _collateral.mint(users[i], 1000000 ether);
            _tokenB.mint(users[i], 1000000e6);
            _tokenC.mint(users[i], 1000000 ether);
            _mockDai.mint(users[i], 1000000 ether);
            IERC20(address(_collateral)).approve(address(_fraxLPToken1Peas), type(uint256).max);
        }
    }

}

    function testPreviewWithdrawInsufficientLiquidity() public {
        vm.startPrank(users[1]);
        _fraxLPToken1Peas.borrowAsset(100_000 ether, 100000 ether, users[1]);

        vm.startPrank(users[2]);
        _fraxLPToken1Peas.borrowAsset(99_000 ether, 100000 ether, users[1]);
        
        // Try to preview withdraw more than available
        uint256 shares = _fraxLPToken1Peas.previewRedeem(_fraxLPToken1Peas.balanceOf(address(this)));
        assert(shares > 0);
        
        // This will revert despite successful preview
        vm.expectRevert();
        _fraxLPToken1Peas.redeem(shares, address(this), address(this));
    }

function testMaxWithdrawalZeroOutput() public {
    // deposit some collateral for user 1
    vm.startPrank(users[1]);
    _fraxLPToken1Peas.borrowAsset(100000 ether, 100000 ether, users[1]);

    vm.startPrank(users[2]);
    _fraxLPToken1Peas.borrowAsset(100000 ether, 100000 ether, users[1]);

    vm.startPrank(address(this));

    uint256 lendingAssetVaultToAssetAmount = _lendingAssetVault.totalAvailableAssetsForVault(address(_fraxLPToken1Peas));
     assertEq(lendingAssetVaultToAssetAmount, 0);
    uint256 maxWithdraw = _fraxLPToken1Peas.maxWithdraw(address(this));
    assertEq(maxWithdraw, 0); // Incorrectly returns 0 when all assets are borrowed
}

Mitigation

Since the team intention is to simulate ERC4626, which is an attempt to comply with the standards, consider following the standard.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

017.md

017.md

Incorrect implementation in fraxlend pair violating standard causes DOS

Summary

Root Cause

Impact

PoC

Mitigation

Files

017.md

Latest commit

History

017.md

File metadata and controls

Incorrect implementation in fraxlend pair violating standard causes DOS

Summary

Root Cause

Impact

PoC

Mitigation