Papr contest - HollaDieWaldfee's results

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L264-L294 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L297-L345 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L326 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L290-L293 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L283-L284

Vulnerability details

Impact

This report deals with the PaprController.purchaseLiquidationAuctionNFT function (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L264-L294).

If there are mutliple auctions going on at the same time, leaving 0 NFTs in the vault, the first auction that is finished will cause the debt of the vault to be set to 0.

Since the debt is reset, all earnings from the future NFT sales will go to the borrower (after deducting liquidation fees).

So if there are multiple NFT auctions going on at the same time (which is totally possible) not all debt might be repaid even though there are more earnings available from future sales.

This is a problem for two reasons:

1. Debt is not repaid as best as possible. More debt could be repaid than actually is. This causes higher interest rates and thereby other borrowers need to pay more interest than they otherwise would have to.
2. Borrowers that cannot repay their debt and get liquidated receive earnings from auctions without their debt being fully repaid.

How big of an issue this second point is depends on the value of parameters (e.g. auctionDecayPeriod, liquidationAuctionMinSpacing), current market conditions, the liquidity of the NFTs used as collateral.

E.g. if all auctions are completed before liquidationAuctionMinSpacing is reached, there is no issue.

However the protocol does not prohibit auctions from overlapping and so if auctions take a long time (Time Weighted Average Price (TWAP) well above market price) and borrowers intentionally choose collateral that is hard to sell, it can become a loophole that borrowers very intentionally use to lose less if they get liquidated.

In principle (although very unlikely) it is even possible that borrowers can earn money from doing this instead of only alleviating their losses due to liquidation.

Proof of Concept

1. The borrower adds two NFTs as collateral (NFT1 and NFT2).
2. The borrower increases his debt
3. Market conditions change and the collateral is not enough to cover the debt so a liquidator calls PaprController.startLiquidationAuction (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L297-L345) to start an auction with NFT1
4. For some reason (examples explained in "Impact" section above) the auction does not finish before the liquidationAuctionMinSpacing is reached (currently 2 days)
5. The liquidator now calls PaprController.startLiquidationAuction to start an auction with NFT2. At this point info.count will be equal to 0 (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L326) since it was decremented two times.
6. One of the two NFTs, say NFT1, is now bought by someone calling PaprController.purchaseLiquidationAuctionNFT. The variable bool isLastCollateral is true since there are no more NFTs in the vault. So if the price of the NFT sale is not enough to cover the debt, remaining is greater than 0. So all remaining debt will be reduced to 0 (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L290-L293).
7. When NFT2 is sold the whole sale price will count as excess because debt is 0 and uint256 neededToSaveVault = maxDebtCached > debtCached ? 0 : debtCached - maxDebtCached; evaluates to 0. The sale price will be paid to the borrower after deducting liquidation fees (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L283-L284).

Tools Used

VSCode

Recommended Mitigation Steps

If there is debt left without any NFTs remaining, the debt of the vault should only be set to 0 if there are no more auctions running.

You can implement this by keeping track of running auctions in the IPaprController.VaultInfo struct (you must decide on the size of the uint):

        struct VaultInfo {
                /// @dev number of collateral tokens in the vault
                uint16 count;
                /// @dev start time of last auction the vault underwent, 0 if no auction has been started
                uint40 latestAuctionStartTime;
                /// @dev debt of the vault, expressed in papr token units
                uint200 debt;
+               uint auctionsCount;
          }

Increment this by one when an auction is started in PaprController.startLiquidationAuction:

         info.latestAuctionStartTime = uint40(block.timestamp);
         info.count -= 1;
+        info.auctionsCount += 1;
 
         emit RemoveCollateral(account, collateral.addr, collateral.id);

Decrement this by one when a purchase is made in PaprController.purchaseLiquidationAuctionNFT and check that auctionsCount==0 before resetting debt:

@@ -279,6 +279,7 @@ contract PaprController is
         uint256 price = _purchaseNFTAndUpdateVaultIfNeeded(auction, maxPrice, sendTo);
         uint256 excess = price > neededToSaveVault ? price - neededToSaveVault : 0;
         uint256 remaining;
+        _vaultInfo[auction.nftOwner][auction.auctionAssetContract].auctionsCount -= 1;
 
         if (excess > 0) {
             remaining = _handleExcess(excess, neededToSaveVault, debtCached, auction);
@@ -287,7 +288,7 @@ contract PaprController is
             remaining = debtCached - price;
         }
 
-        if (isLastCollateral && remaining != 0) {
+        if (isLastCollateral && remaining != 0 && _vaultInfo[auction.nftOwner][auction.auctionAssetContract].auctionsCount == 0) {
             /// there will be debt left with no NFTs, set it to 0
             _reduceDebtWithoutBurn(auction.nftOwner, auction.auctionAssetContract, remaining);
         }

Lines of code

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

Vulnerability details

Impact

There are two ways to reduce debt.

The first is by calling PaprController.reduceDebt (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L148) which burns papr token from msg.sender directly.
The second is the PaprController.buyAndReduceDebt function (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L208) which swaps the underlying token for papr token and then burns the papr token.

Both ways internally call PaprController._reduceDebt (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L481).

The PaprController._reduceDebt function reverts if the amount parameter is bigger than the actual debt in the vault.

This is a problem for two reasons:

1. When a user wants to pay all his debt using the PaprController.buyAndReduceDebt function he must perform complex math in order to calculate the amount of input token that results in the exact amount of papr token

2. An attacker can front-run a transaction that repays all debt by paying back 1 Wei. The transaction to pay back the whole debt then reverts

The first issue is more a usability issue than a security issue.
However the second issue can be used to DOS the application. Especially if other protocols integrate with the papr protocol and are not flexible enough to pay back amounts that are smaller than the maximum.

Proof of Concept

PaprController._reduceDebt calls PaprController._reduceDebtWithoutBurn (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L486-L489):

function _reduceDebtWithoutBurn(address account, ERC721 asset, uint256 amount) internal {
    _vaultInfo[account][asset].debt = uint200(_vaultInfo[account][asset].debt - amount);
    emit ReduceDebt(account, asset, amount);
}

This reverts if amount > _vaultInfo[account][asset].

Tools Used

VSCode

Recommended Mitigation Steps

If PaprController._reduceDebt is called with an amount > _vaultInfo[account][asset].debt, then amount should be set to _vaultInfo[account][asset].debt.

So PaprController._reduceDebt should look like this:

function _reduceDebt(address account, ERC721 asset, address burnFrom, uint256 amount) internal {
    if (amount > _vaultInfo[account][asset].debt) {
        amount = _vaultInfo[account][asset].debt;
    }
    _reduceDebtWithoutBurn(account, asset, amount);
    PaprToken(address(papr)).burn(burnFrom, amount);
}

Lines of code

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

Vulnerability details

Impact

The PaprController.buyAndReduceDebt function (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L208-L232) should work like this:

1. msg.sender swaps some amount of the underlying token for papr token
2. This amount of papr token is used to repay debt for the address in the account parameter

msg.sender and account can be different addresses such that one can repay anyone's debt.

However there is a mistake in the function which leads to this behavior:

1. msg.sender swaps some amount of the underlying token for papr token
2. The papr token is sent to the account address
3. The papr token is burnt from the msg.sender
4. The amount of papr token burnt from the msg.sender is used to pay back the debt of the account address

The issue is that the swapped papr token are sent to account but the papr token are burnt from msg.sender.

In the best scenario when calling this function, the msg.sender does not have enough papr token to burn so the function call reverts.

In the scenario that is worse, the msg.sender has enough papr token to be burnt.
So the account address receives the swapped papr token and the debt of account is paid as well by the msg.sender.

Thereby the msg.sender pays double the amount he wants to.
Once by swapping his underlying tokens for papr.
The second time because his papr token are burnt.

Proof of Concept

The PaprController.buyAndReduceDebt function (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L208-L232) calls UniswapHelpers.swap (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/libraries/UniswapHelpers.sol#L31-L61):

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

The second parameter which has the value account is the recipient of the swap.
The last parameter which is msg.sender is the address paying the input amount for the swap.

So the msg.sender pays some amount of underlying and the papr that the underlying is swapped for is sent to the account.

But then the debt of account is reduced by burning papr token from msg.sender:

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

However the papr token from the swap were received by account. So the msg.sender pays twice and account receives twice.

Tools Used

VSCode

Recommended Mitigation Steps

The swapped papr token should be sent to the msg.sender instead of account such that they can then be burnt from msg.sender.

In order to achieve this, a single line in PaprController.buyAndReduceDebt must be changed:

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

Judge has assessed an item in Issue #172 as M risk. The relevant finding follows:

[L-02]

Papr Low Risk and Non-Critical Issues

Summary

[L-00] Check that targetMarkRatioMax > targetMarkRatioMin

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/UniswapOracleFundingRateController.sol#L38-L39

In the constructor of UniswapOracleFundingRateController it should be checked that targetMarkRatioMax > targetMarkRatioMin as otherwise the values do not make sense and cause issues in the calculation of the funding rate.

[L-01] Liquidation Fees are burnt and cannot be withdrawn by admin

The Whitepaper (https://backed.mirror.xyz/8SslPvU8of0h-fxoo6AybCpm51f30nd0qxPST8ep08c) states that:

If there is excess papr, a liquidation fee is subtracted and given to the protocol (added to an insurance fund) and the remainder is used to further reduce the borrowers debt

In order to make use of the Liquidation fees and to transfer them to the insurance fund, the PaprController has the sendPaprFromAuctionFees function (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L382-L384).

However the liquidation fees are instantly burnt once they are calculated after the NFT purchase:
https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L540

So there are no fees to be withdrawn.

In order to fix this, just remove the above line that burns the fees.

[L-02] buyAndReduceDebt function reverts if swap fees are used

The PaprController.buyAndReduceDebt function mistakenly sends the swap fees (which is an optional feature the msg.sender may or may not use) from the PaprController contract to the swapFeeTo address:
https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L225-L227

This is wrong. The swap fees should be transferred from msg.sender to the swapFeeTo address.

The PaprController does not hold any underlying tokens and so if swap fees are supposed to be used, the function call will revert, making the optional swap fee feature unusable.

To fix this, change the line that transfers the tokens to:

underlying.transferFrom(msg.sender, params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);

[N-00] Function state mutability can be restricted to pure

In Solidity, functions should make use of the most restrictive state mutability modifier possible.

The following functions can be pure:

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/NFTEDA/libraries/EDAPrice.sol#L10-L23

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/libraries/OracleLibrary.sol#L34-L53

[N-01] Function state mutability can be restricted to view

In Solidity, functions should make use of the most restrictive state mutability modifier possible.

The following functions can be view:

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/ReservoirOracleUnderwriter.sol#L64-L117

[N-03] onERC721Received function: check amount > 0 instead of minOut > 0

The PaprController.onERC721Received function allows to optionally call _increaseDebtAndSell or _increaseDebt.
_increaseDebtAndSell is called when request.swapParams.minOut > 0 (https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L170).

Instead of request.swapParams.minOut > 0 it should be checked that request.swapParams.amount > 0.
This is because minOut should be an optional parameter that can be left at zero whereas amount is a required parameter for the swap.