Papr contest - noot's results

Lines of code

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

Vulnerability details

Impact

PaprController.buyAndReduceDebt() does not validate params.swapFeeBips, which can be set to any value by the user. This allows the user to arbitrarily manipulate the amount of underlying token transferred to them, and thus can drain the contract.

Proof of Concept

The function buyAndRedudeDebt() is callable by anyone, and is as follows:

function buyAndReduceDebt(address account, ERC721 collateralAsset, IPaprController.SwapParams calldata params)
        external
        override
        returns (uint256)
{
        bool hasFee = params.swapFeeBips != 0;

        (uint256 amountOut, uint256 amountIn) = UniswapHelpers.swap(
            pool,
            account,
            token0IsUnderlying,
            params.amount,
            params.minOut,
            params.sqrtPriceLimitX96,
            abi.encode(msg.sender)
        );

        if (hasFee) {
            underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);
        }

        _reduceDebt({account: account, asset: collateralAsset, burnFrom: msg.sender, amount: amountOut});

        return amountOut;
}

To successfully drain the contract of underlying via underlying.transfer():

1. The attacker sets params.swapFeeTo to be their own address, and params.swapFeeBips to be a large value such that amountIn * params.swapFeeBips / BIPS_ONE >= underlying.balanceOf(address(this)). Note that the caller has control over amountIn via params.amount and params.minOut.
2. To pass _reduceDebt, the attacker also needs to ensure that amountOut <= _vaultInfo[account][collateralAsset].debtand also that amountOut <= the Papr balance of msg.sender. To ensure they have enough debt and also have Papr, the attacker can call increaseDebt after depositing any valid ERC721 via addCollateral -> _addCollateralToVault(). Also note that collateralAsset is not validated, so the attacker can also pass any ERC721 that they had previously deposited.
3. To pass the uniswapV3SwapCallback(), the attacker also needs to ensure that they have >=amountToPay underlying token. However, the amountToPay is also manipulable by the caller via params.amount in buyAndReduceDebt; they can just set some small value to be transferred out of their account.
4. The attacker calls buyAndReduceDebt with parameters that satisfy the above constraints, which will then transfer the desired amount of underlying to the attacker.

Tools Used

n/a

Recommended Mitigation Steps

Validate params.swapFeeBips such that the amount transferred out of the user's account in uniswapV3SwapCallback is proportional to it, or pass it to _reduceDebt() and increase the user's debt proportionally to params.swapFeeBips.

_purchaseNFTAndUpdateVaultIfNeeded should be called first in purchaseLiquidationAuctionNFT

In purchaseLiquidationAuctionNFT(), various calculations are done before _purchaseNFTAndUpdateVaultIfNeeded() is called. However, none of these calculated values are passed to _purchaseNFTAndUpdateVaultIfNeeded(), which may revert. The call to _purchaseNFTAndUpdateVaultIfNeeded() should be the first thing to happen in the function to save users gas in case of reverts.

removeCollateral should accept accept ERC721 address and array of IDs instead of array of Collateral

This function requires all collateral addresses to be the same. To remove the address check which happens on every iteration >0, this function should instead accept the collateral address and array of IDs. This can also remove the if (i == 0) check.

timestamp check in startLiquidationAuction is unneeded

There is no way to end an auction once it starts (apart from purchasing the NFT), thus calling startLiquidationAuction multiple times will revert in the _startAuction call. The timestamp check inside the function:

        if (block.timestamp - info.latestAuctionStartTime < liquidationAuctionMinSpacing) {
            revert IPaprController.MinAuctionSpacing();
        }

effectively does nothing, as info.latestAuctionStartTime will always be 0 on the first call to start the auction, and if it's set, the call will revert during _startAuction.

newDebt check should be executed earlier in _increaseDebt()

The check for newDebt inside _increaseDebt is as follows:

        if (newDebt >= 1 << 200) revert IPaprController.DebtAmountExceedsUint200();

This line should be moved directly under the line which calculates newDebt to save gas in case of reverts.