Revert Lend - t4sk's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L304-L305

Vulnerability details

Impact

V3Oracle._getReferenceTokenPriceX96 reverts, not able to return a price

Proof of Concept

chainlinkPriceX96 is calculated by the following code.

chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96
                / (10 ** feedConfig.tokenDecimals);

The numerator can overflow when reference token has 18 decimals.

Math

        10 ** 18 * chainlinkPriceX96 * Q96 <= 2**256
        10 ** 18 * chain link price * Q96 * Q96 <= 2**256
        10 ** 18 * chain link price <= 2**(256 - 2 * 96)
        chain link price <= 2 ** 64 / 10 ** 18 <= 19

This shows that if reference token has 18 decimals, the price oracle can only return a price if the token price is <= 19

Second test fails. It uses DAI (which has 18 decimals) as a reference token

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

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

// forge test --fork-url $FORK_URL --match-path test/dev.sol -vvv
contract V3OracleTest is Test {
    uint256 constant Q96 = 2 ** 96;
    address constant USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
    address constant DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
    address constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address constant CHAINLINK_USDC_USD =
        0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6;
    address constant CHAINLINK_DAI_USD =
        0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9;
    address constant CHAINLINK_ETH_USD =
        0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;

    mapping(address => address) private feeds;

    function setUp() public {
        feeds[USDC] = CHAINLINK_USDC_USD;
        feeds[DAI] = CHAINLINK_DAI_USD;
        feeds[WETH] = CHAINLINK_ETH_USD;
    }

    function test() public {
        uint256 price = get(WETH, 18, USDC, 6);
        console.log("price", price);
    }

    function testFail() public {
        // price overflows
        uint256 price = get(WETH, 18, DAI, 18);
        console.log("price", price);
    }

    function get(
        address token,
        uint256 tokenDecimals,
        address referenceToken,
        uint256 referenceTokenDecimals
    ) public view returns (uint256) {
        uint256 chainlinkPriceX96 = _getChainlinkPriceX96(token, referenceToken);
        uint256 chainlinkReferencePriceX96 =
            _getChainlinkPriceX96(referenceToken, referenceToken);

        // console.log("chainlink price", chainlinkPriceX96 / Q96);
        // console.log("ref price", chainlinkReferencePriceX96 / Q96);

        // 10 ** 18 * chain link price * Q96 * Q96 <= 2**256
        // 10 ** 18 * chain link price <= 2**(256 - 2 * 96)
        // chain link price <= 2 ** 64 / 10 ** 18 <= 19
        chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96
            * Q96 / chainlinkReferencePriceX96 / (10 ** tokenDecimals);

        return chainlinkPriceX96;
    }

    function _getChainlinkPriceX96(address token, address referenceToken)
        internal
        view
        returns (uint256)
    {
        if (token == referenceToken) {
            return Q96;
        }

        (, int256 answer,, uint256 updatedAt,) =
            AggregatorV3Interface(feeds[token]).latestRoundData();
        require(answer >= 0, "answer < 0");

        // ETH, USDC, DAI all have 8 decimals
        return uint256(answer) * Q96 / (10 ** 8);
    }
}

interface AggregatorV3Interface {
    function latestRoundData()
        external
        view
        returns (
            uint80 roundId,
            int256 answer,
            uint256 startedAt,
            uint256 updatedAt,
            uint80 answeredInRound
        );

    function decimals() external view returns (uint8);
}

Tools Used

Manual review, Foundry, python

Recommended Mitigation Steps

Rearrange multiplications and divisions. This will make the price less accurate.

        if (referenceTokenDecimals >= tokenDecimals) {
            chainlinkPriceX96 = chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96 * (10 ** (referenceTokenDecimals - tokenDecimals))
        } else {
            chainlinkPriceX96 = chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96 / (10 ** (tokenDecimals - referenceTokenDecimals))
        }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L143-L145

Vulnerability details

Impact

Asymmetric calculation of price difference

Proof of Concept

Price difference is calculated in 2 ways depending on whether price > verified price or not.

If price > verified price, this is the equation.

(price - verified price) / price

Otherwise price is calculated with this equation

(verified price - price) / verified price

When the 2 equations above are graphed with price = horizontal axis, we get 2 different curves.

https://www.desmos.com/calculator/nixha3ojz6

The first equation produces a asymptotic curve. (shown in red) The second equation produces a linear curve. (shown in green) Therefore the rate at which the price difference changes is different depending on whether price > verified price or not.

Example

Price difference of +1 or -1 from verified price are not symmetric

# p < v
v = 2
p = 1
d = (v - p) / v
print(d)
# output is 0.5

# p > v
v = 2
p = 3
d = (p - v) / p
print(d)
# output is 0.33333

Tools Used

Manual review, desmos graphing calculator and python

Recommended Mitigation Steps

Use a different equation to check price difference (shown in blue)

|price - verified price| / verified price <= max difference

Assuming verifyPriceX96 > 0

        uint256 diff = priceX96 >= verifyPriceX96
            ? (priceX96 - verifyPriceX96) * 10000
            : (verifyPriceX96 - priceX96) * 10000;
        
        require(diff / verifyPriceX96 <= maxDifferenceX1000)

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L964-L970

Vulnerability details

Impact

Repayment with asset can leave dust and clean up function is not called.

Proof of Concept

1. User submits transaction to repay loan in full amount
2. Because transactions take some time to process and the loan debt is always increasing, by the time user's transaction is executed, the user did not pay the full amount needed to unlock his collateral.

POC shows user's debt is not fully repaid.

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {V3Oracle} from "../../src/V3Oracle.sol";
import {V3Vault, IPermit2, INonfungiblePositionManager} from "../../src/V3Vault.sol";
import {InterestRateModel} from "../../src/InterestRateModel.sol";

address constant TOKEN_0 = address(10);
address constant TOKEN_1 = address(11);

// forge test --match-path test/integration/RepayDust.t.sol -vvv
contract RepayDustTest is Test {
    uint256 private constant Q96 = 2 ** 96;
    uint256 private constant Q32 = 2 ** 32;
    uint32 private constant MAX_COLLATERAL_FACTOR_X32 = uint32(Q32 * 90 / 100); // 90%

    ERC20 private asset;
    INonfungiblePositionManager private nftPositionManager;
    InterestRateModel private interestRateModel;
    V3Oracle private oracle = V3Oracle(address(123));
    IPermit2 private permit2;
    V3Vault private vault;
    uint256 private constant TOKEN_ID = 999;

    function setUp() public {
        asset = new ERC20("token", "TOKEN", 6);
        nftPositionManager = INonfungiblePositionManager(address(new MockNftPositionManager()));
        // 0% base rate - 5% multiplier - after 80% - 109% jump multiplier (like in compound v2 deployed)  (-> max rate 25.8% per year)
        interestRateModel = new InterestRateModel(0, Q96 * 5 / 100, Q96 * 109 / 100, Q96 * 80 / 100);
        vault = new V3Vault("test", "TEST", address(asset), nftPositionManager, interestRateModel, oracle, permit2);

        vault.setLimits(0, 15000 * 1e6, 15000 * 1e6, 2000 * 1e6, 1000 * 1e6);
        vault.setTokenConfig(TOKEN_0, MAX_COLLATERAL_FACTOR_X32, type(uint32).max);
        vault.setTokenConfig(TOKEN_1, MAX_COLLATERAL_FACTOR_X32, type(uint32).max);

        asset.mint(address(this), 2000 * 1e6);
        asset.approve(address(vault), type(uint256).max);

        // Supply token to borrow
        vault.deposit(1000 * 1e6, address(this));

        // Set tokenOwner to this contract
        vm.prank(address(nftPositionManager));
        vault.onERC721Received(address(0), address(this), TOKEN_ID, "");

        // Mock oracle collateral value
        vm.mockCall(
            address(oracle),
            abi.encodeCall(V3Oracle.getValue, (TOKEN_ID, address(asset))),
            abi.encode(uint256(1e6 * 1e6), uint256(1), uint256(0), uint256(0))
        );
    }

    function test_repay_dust() public {
        // 1. User borrows
        vault.borrow(TOKEN_ID, 1000 * 1e6);

        skip(1 days);

        // 2. User repays debt0
        (uint256 debt0, , , , ) = vault.loanInfo(TOKEN_ID);
        console.log("Debt", debt0);

        asset.approve(address(vault), type(uint256).max);

        // 3. Simulate transaction pending time 
        skip(10);

        // 4. Actual debt needed to repay all
        (uint256 debt1, , , , ) = vault.loanInfo(TOKEN_ID);
        console.log("Debt", debt1);

        vault.repay(TOKEN_ID, debt0, false);

        // Dust remains, user could not repay all
        (uint256 debtShares) = vault.loans(TOKEN_ID);
        console.log("Debt shares", debtShares);
    }
}

contract MockNftPositionManager {
    function factory() external view returns (address) {
        return address(1111);
    }

    function positions(uint256 tokenId)
        external
        view
        returns (
            uint96 nonce,
            address operator,
            address token0,
            address token1,
            uint24 fee,
            int24 tickLower,
            int24 tickUpper,
            uint128 liquidity,
            uint256 feeGrowthInside0LastX128,
            uint256 feeGrowthInside1LastX128,
            uint128 tokensOwed0,
            uint128 tokensOwed1
        )
    {
        return (0, address(0), TOKEN_0, TOKEN_1, 0, 0, 0, 0, 0, 0, 0, 0);
    }
}

contract ERC20 {
    event Transfer(address indexed from, address indexed to, uint256 value);
    event Approval(address indexed owner, address indexed spender, uint256 value);

    uint256 public totalSupply;
    mapping(address => uint256) public balanceOf;
    mapping(address => mapping(address => uint256)) public allowance;
    string public name;
    string public symbol;
    uint8 public decimals;

    constructor(string memory _name, string memory _symbol, uint8 _decimals) {
        name = _name;
        symbol = _symbol;
        decimals = _decimals;
    }

    function transfer(address recipient, uint256 amount) external returns (bool) {
        balanceOf[msg.sender] -= amount;
        balanceOf[recipient] += amount;
        emit Transfer(msg.sender, recipient, amount);
        return true;
    }

    function approve(address spender, uint256 amount) external returns (bool) {
        allowance[msg.sender][spender] = amount;
        emit Approval(msg.sender, spender, amount);
        return true;
    }

    function transferFrom(address sender, address recipient, uint256 amount) external returns (bool) {
        allowance[sender][msg.sender] -= amount;
        balanceOf[sender] -= amount;
        balanceOf[recipient] += amount;
        emit Transfer(sender, recipient, amount);
        return true;
    }

    function _mint(address to, uint256 amount) internal {
        balanceOf[to] += amount;
        totalSupply += amount;
        emit Transfer(address(0), to, amount);
    }

    function _burn(address from, uint256 amount) internal {
        balanceOf[from] -= amount;
        totalSupply -= amount;
        emit Transfer(from, address(0), amount);
    }

    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) external {
        _burn(from, amount);
    }
}

forge test --match-path test/integration/RepayDust.t.sol -vvv

[PASS] test_repay_dust() (gas: 206781)
Logs:
  Debt 1000706366
  Debt 1000706448
  Debt shares 82

Tools Used

Manual review and Foundry

Recommended Mitigation Steps

Allow input amount to be type(uint).max. Interpret type(uint).max to mean that the user wishes to repay all debt.

       if (isShare) {
            shares = min(amount, currentShares);
            assets = _convertToAssets(amount, newDebtExchangeRateX96, Math.Rounding.Up);
        } else {
            if (amount == type(uint).max) {
                assets = _convertToAssets(currentShares, newDebtExchangeRateX96, Math.Rounding.Up);
                shares = currentShares;
            } else {
                assets = amount;
                shares = _convertToShares(amount, newDebtExchangeRateX96, Math.Rounding.Down);
            }
        }

Assessed type

Math