Papr contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

Users will lose collateral NFTs when they are transferred to PaprController by an approved address or an operator.

Proof of Concept

The PaprController allows users to deposit NFTs as collateral to borrow Papr tokens. One of the way of depositing is by transferring an NFT to the contract directly via a call to safeTransferFrom: the contract implements the onERC721Received hook that will handle accounting of the transferred NFT (PaprController.sol#L159). However, the hook implementation uses a wrong argument to identify token owner: the first argument, which is used by the contract to identify token owner, is the address of the safeTransferFrom function caller, which may be an approved address or an operator. The actual owner address is the second argument (ERC721.sol#L436):

try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {

Thus, when an NFT is sent by an approved address or an operator, it'll be deposited to the vault of the approved address or operator:

// test/paprController/OnERC721ReceivedTest.sol

function testSafeTransferByOperator_AUDIT() public {
    address operator = address(0x12345);

    vm.prank(borrower);
    nft.setApprovalForAll(operator, true);

    vm.prank(operator);
    nft.safeTransferFrom(borrower, address(controller), collateralId, abi.encode(safeTransferReceivedArgs));

    // NFT was deposited to the operator's vault.
    IPaprController.VaultInfo memory vaultInfo = controller.vaultInfo(operator, collateral.addr);
    assertEq(vaultInfo.count, 1);

    // Borrower has 0 tokens in collateral.
    vaultInfo = controller.vaultInfo(borrower, collateral.addr);
    assertEq(vaultInfo.count, 0);
}

function testSafeTransferByApproved_AUDIT() public {
    address approved = address(0x12345);

    vm.prank(borrower);
    nft.approve(approved, collateralId);

    vm.prank(approved);
    nft.safeTransferFrom(borrower, address(controller), collateralId, abi.encode(safeTransferReceivedArgs));

    // NFT was deposited to the approved address's vault.
    IPaprController.VaultInfo memory vaultInfo = controller.vaultInfo(approved, collateral.addr);
    assertEq(vaultInfo.count, 1);

    // Borrower has 0 tokens in collateral.
    vaultInfo = controller.vaultInfo(borrower, collateral.addr);
    assertEq(vaultInfo.count, 0);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

--- a/src/PaprController.sol
+++ b/src/PaprController.sol
@@ -156,7 +156,7 @@ contract PaprController is
     /// @param _id the id of the NFT
     /// @param data encoded IPaprController.OnERC721ReceivedArgs
     /// @return selector indicating succesful receiving of the NFT
-    function onERC721Received(address from, address, uint256 _id, bytes calldata data)
+    function onERC721Received(address, address from, uint256 _id, bytes calldata data)
         external
         override
         returns (bytes4)

Lines of code

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

Vulnerability details

Impact

Since there's no gap between the maximal LTV and the liquidation LTV, user positions may be liquidated as soon as maximal debt is taken, without leaving room for collateral and Papr token prices fluctuations. Users have no chance to add more collateral or reduce debt before being liquidated. This may eventually create more uncovered and bad debt for the protocol.

Proof of Concept

The protocol allows users to take debt up to the maximal debt, including it (PaprController.sol#L471):

if (newDebt > max) revert IPaprController.ExceedsMaxDebt(newDebt, max);

However, a positions becomes liquidable as soon as user's debt reaches user's maximal debt (PaprController.sol#L317):

if (info.debt < _maxDebt(oraclePrice * info.count, cachedTarget)) {
    revert IPaprController.NotLiquidatable();
}

Moreover, the same maximal debt calculation is used during borrowing and liquidating, with the same maximal LTV (PaprController.sol#L556-L559):

function _maxDebt(uint256 totalCollateraValue, uint256 cachedTarget) internal view returns (uint256) {
    uint256 maxLoanUnderlying = totalCollateraValue * maxLTV;
    return maxLoanUnderlying / cachedTarget;
}

Even though different price kinds are used during borrowing and liquidations (LOWER during borrowing, TWAP during liquidations), the price can in fact match (ReservoirOracleUnderwriter.sol#L11):

/// @dev LOWER is the minimum of SPOT and TWAP

Which means that the difference in prices doesn't always create a gap in maximal and liquidation LTVs.

The combination of these factors allows users to take maximal debts and be liquidated immediately, in the same block. Since liquidations are not beneficial for lending protocols, such heavy penalizing of users may harm the protocol and increase total uncovered debt, and potentially lead to a high bad debt.

// test/paprController/IncreaseDebt.t.sol

event RemoveCollateral(address indexed account, ERC721 indexed collateralAddress, uint256 indexed tokenId);

function testIncreaseDebtAndBeLiquidated_AUDIT() public {
    vm.startPrank(borrower);
    nft.approve(address(controller), collateralId);
    IPaprController.Collateral[] memory c = new IPaprController.Collateral[](1);
    c[0] = collateral;
    controller.addCollateral(c);

    // Calculating the max debt for the borrower.
    uint256 maxDebt = controller.maxDebt(1 * oraclePrice);

    // Taking the maximal debt.
    vm.expectEmit(true, true, false, true);
    emit IncreaseDebt(borrower, collateral.addr, maxDebt);
    controller.increaseDebt(borrower, collateral.addr, maxDebt, oracleInfo);
    vm.stopPrank();

    // Making a TWAP price that's identical to the LOWER one.
    priceKind = ReservoirOracleUnderwriter.PriceKind.TWAP;
    ReservoirOracleUnderwriter.OracleInfo memory twapOracleInfo = _getOracleInfoForCollateral(nft, underlying);

    // The borrower is liquidated in the same block.
    vm.expectEmit(true, true, false, false);
    emit RemoveCollateral(borrower, collateral.addr, collateral.id);
    controller.startLiquidationAuction(borrower, collateral, twapOracleInfo);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a liquidation LTV that's bigger than the maximal borrow LTV; positions can only be liquidated after reaching the liquidation LTV. This will create a room for price fluctuations and let users increase their collateral or decrease debt before being liquidating. Alternatively, consider liquidating positions only after their debt has increased the maximal one:

--- a/src/PaprController.sol
+++ b/src/PaprController.sol
@@ -314,7 +314,7 @@ contract PaprController is

         uint256 oraclePrice =
             underwritePriceForCollateral(collateral.addr, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo);
-        if (info.debt < _maxDebt(oraclePrice * info.count, cachedTarget)) {
+        if (info.debt <= _maxDebt(oraclePrice * info.count, cachedTarget)) {
             revert IPaprController.NotLiquidatable();
         }

Lines of code

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

Vulnerability details

Impact

Since PaprController is not designed to hold any underlying tokens, calling buyAndReduceDebt with a swap fee set will result in a revert. The function can also be used to transfer out any underlying tokens sent to the contract mistakenly.

Proof of Concept

PaprController implements the buyAndReduceDebt function, which allows users to buy Papr tokens for underlying tokens and burn them to reduce their debt (PaprController.sol#L208). Optionally, the function allows the caller to specify a swap fee: a fee that's collected from the caller. However, in reality, the fee is collected from PaprController itself: transfer instead of transferFrom is called on the underlying token (PaprController.sol#L225-L227):

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

This scenario is covered by the testBuyAndReduceDebtReducesDebt test (BuyAndReduceDebt.t.sol#L12), however the fee is not actually set in the test:

// Fee is initialized but not set.
uint256 fee;
underlying.approve(address(controller), underlyingOut + underlyingOut * fee / 1e4);
swapParams = IPaprController.SwapParams({
    amount: underlyingOut,
    minOut: 1,
    sqrtPriceLimitX96: _maxSqrtPriceLimit({sellingPAPR: false}),
    swapFeeTo: address(5),
    swapFeeBips: fee
});

If fee is set in the test, the test wil revert with an "Arithmetic over/underflow" error:

--- a/test/paprController/BuyAndReduceDebt.t.sol
+++ b/test/paprController/BuyAndReduceDebt.t.sol
@@ -26,7 +26,7 @@ contract BuyAndReduceDebt is BasePaprControllerTest {
         IPaprController.VaultInfo memory vaultInfo = controller.vaultInfo(borrower, collateral.addr);
         assertEq(vaultInfo.debt, debt);
         assertEq(underlyingOut, underlying.balanceOf(borrower));
-        uint256 fee;
+        uint256 fee = 1e3;
         underlying.approve(address(controller), underlyingOut + underlyingOut * fee / 1e4);
         swapParams = IPaprController.SwapParams({
             amount: underlyingOut,

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

--- a/src/PaprController.sol
+++ b/src/PaprController.sol
@@ -223,7 +223,7 @@ contract PaprController is
         );

         if (hasFee) {
-            underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);
+            underlying.safeTransferFrom(msg.sender, params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);
         }

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

[L-01] Loss of precision in oneToOneSqrtRatio

Targets

• https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/libraries/UniswapHelpers.sol#L111

Impact

A Uniswap pool may not be initialized at exact 1:1 price when an underlying token's decimals are not 18.

Proof of Concept

When a PaprController is deployed and initialized it deploys a Uniswap V3 pool with Papr and an underlying token (PaprController.sol#L92). The pool will serve as the market price discovery mechanism of Papr: changes in price of Papr in the pool will affect the interest rate of Papr. Initially, the price of Papr is set to 1:1 with the underlying token (PaprController.sol#L85-L90), however the calculation of such a price is not precise: the numerator is shifted left by 96 bits, instead of 192, which reduces the precision of the square root operation (UniswapHelpers.sol#L110-L112):

function oneToOneSqrtRatio(uint256 token0ONE, uint256 token1ONE) internal pure returns (uint160) {
    return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
}

For example, when calling this function with token0ONE = 1e18 and token1ONE = 1e6, we get 79228267247129223624114, which is (79228267247129223624114 / 2**96) ** 2 = 1.0000026438309507e-12 and 1 / 1.0000026438309507e-12 = 999997356176.0391, which is not precise 1:1.

Here are more examples:

  Error: 18 and 6
  Error: a == b not satisfied [uint]
    Expected: 79228162514264337593543
      Actual: 79228267247129223624114
  Error: 6 and 18
  Error: a == b not satisfied [uint]
    Expected: 79228162514264337593543950336000000
      Actual: 79228057781537899283318961129827820
  Error: 18 and 8
  Error: a == b not satisfied [uint]
    Expected: 792281625142643375935439
      Actual: 792282497916421281862280
  Error: 8 and 18
  Error: a == b not satisfied [uint]
    Expected: 7922816251426433759354395033600000
      Actual: 7922807523698269125109939133199873

// test/UniswapHelpers.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "forge-std/Test.sol";

import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import "../src/libraries/UniswapHelpers.sol";

contract UniswapHelpersTest is Test {
  function testOneToOneSqrtRatio_AUDIT() public {
    assertEq(
      UniswapHelpers.oneToOneSqrtRatio(1e18, 1e18),
      oneToOneSqrtRatio(1e18, 1e18),
      "18 and 18"
    );

    assertEq(
      UniswapHelpers.oneToOneSqrtRatio(1e18, 1e6),
      oneToOneSqrtRatio(1e18, 1e6),
      "18 and 6"
    );

    assertEq(
      UniswapHelpers.oneToOneSqrtRatio(1e6, 1e18),
      oneToOneSqrtRatio(1e6, 1e18),
      "6 and 18"
    );

    assertEq(
      UniswapHelpers.oneToOneSqrtRatio(1e18, 1e8),
      oneToOneSqrtRatio(1e18, 1e8),
      "18 and 8"
    );

    assertEq(
      UniswapHelpers.oneToOneSqrtRatio(1e8, 1e18),
      oneToOneSqrtRatio(1e8, 1e18),
      "8 and 18"
    );
  }

  function oneToOneSqrtRatio(uint256 token0ONE, uint256 token1ONE) internal pure returns (uint160) {
    return uint160(FixedPointMathLib.sqrt((token1ONE << 192) / token0ONE));
  }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

--- a/src/libraries/UniswapHelpers.sol
+++ b/src/libraries/UniswapHelpers.sol
@@ -6,6 +6,7 @@ import {IUniswapV3Factory} from "v3-core/contracts/interfaces/IUniswapV3Factory.
 import {TickMath} from "fullrange/libraries/TickMath.sol";
 import {FullMath} from "fullrange/libraries/FullMath.sol";
 import {SafeCast} from "v3-core/contracts/libraries/SafeCast.sol";
+import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";

 import {PoolAddress} from "./PoolAddress.sol";

@@ -108,6 +109,6 @@ library UniswapHelpers {
     /// @param token1ONE 10 ** token1.decimals()
     /// @return sqrtRatio at which token0 and token1 are trading at 1:1
     function oneToOneSqrtRatio(uint256 token0ONE, uint256 token1ONE) internal pure returns (uint160) {
-        return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
+        return uint160(FixedPointMathLib.sqrt((token1ONE << 192) / token0ONE));
     }
 }

[L-02] Auction fees cannot be withdrawn since they're always burned

Targets

• https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L382-L384
• https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L540

Impact

The sendPaprFromAuctionFees and burnPaprFromAuctionFees functions of PaprController won't ever be used because the contract doesn't accumulate Papr tokens: excess penalties are always burned during auction.

Proof of Concept

PaprController implements sendPaprFromAuctionFees and burnPaprFromAuctionFees (PaprController.sol#L382-L388) which intended to send or burn Papr tokens collected as auction fees. However, the auction doesn't collect fees (PaprController.sol#L264-L345). It does apply a penalty to the excess amount resulted after selling of an NFT, but the penalty amount is burned immediately (PaprController.sol#L536-L540):

uint256 fee = excess * liquidationPenaltyBips / BIPS_ONE;
uint256 credit = excess - fee;
uint256 totalOwed = credit + neededToSaveVault;

PaprToken(address(papr)).burn(address(this), fee);

The following test demonstrates that no Papr tokens are accumulated by PaprController during a liquidation:

// test/paprController/PurchaseLiquidationAuctionNFT.t.sol

function testAuctionFeesAreBurned_AUDIT() public {
    // add collateral
    uint256 tokenId = collateralId + 5;
    nft.mint(borrower, tokenId);
    vm.stopPrank();
    vm.startPrank(borrower);
    nft.approve(address(controller), tokenId);
    collateral.id = tokenId;
    IPaprController.Collateral[] memory c = new IPaprController.Collateral[](1);
    c[0] = collateral;
    controller.addCollateral(c);
    vm.stopPrank();
    vm.startPrank(purchaser);

    /// https://www.wolframalpha.com/input?i=solve+4+%3D+8.999+*+0.3+%5E+%28x+%2F+86400%29
    vm.warp(block.timestamp + 58187);
    oracleInfo = _getOracleInfoForCollateral(collateral.addr, underlying);
    IPaprController.VaultInfo memory info = controller.vaultInfo(borrower, collateral.addr);

    // Calculating auction penalty from the excess amount.
    uint256 neededToSave = info.debt - controller.maxDebt(oraclePrice);
    uint256 excess = controller.auctionCurrentPrice(auction) - neededToSave;
    uint256 penalty = excess * controller.liquidationPenaltyBips() / 1e4;

    controller.papr().approve(address(controller), auction.startPrice);

    // Expecting the penalty to be burned.
    vm.expectEmit(true, true, false, true);
    emit Transfer(address(controller), address(0), penalty);
    controller.purchaseLiquidationAuctionNFT(auction, auction.startPrice, purchaser, oracleInfo);

    // Papr balance of the controller is 0.
    assertEq(controller.papr().balanceOf(address(controller)), 0);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing or renaming the sendPaprFromAuctionFees and burnPaprFromAuctionFees functions.