Papr contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

may incorrectly returned the Auction funds to the liquidated user

in purchaseLiquidationAuctionNFT(), After someone purchases the auction NFT, the amount of the auction received will be distributed. In the existing logic, when the amount of the auction is paid off the outstanding amount, but still not enough, but if it is already the last collateral, then the remaining outstanding amount will be directly cleared 0, use _reduceDebtWithoutBurn(remaining), if it is not the last collateral will not be cleared 0.

But here is a situation not considered, is to determine whether the last collateral is by _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count, if 0 is the last collateral But a user's NFT, it is possible to have more than one auction at the same time, as long as the second auction time is greater than liquidationAuctionMinSpacing, you can carry out two auction at the same time Thus, the first auction has a purchase, then the amount of debt will be cleared to 0, then the second auction has a purchase, as the amount of debt is 0, so it will be transferred directly to the liquidated person

Proof of Concept

Here is an example: Suppose alice current debt: 100, she has two NFT[1,2]

1. NFT[1] is liquidated, then: vaultInfo[auction.nftOwner][auction.auctionAssetContract].count = 1
2. assume that no one has bought, time has passed liquidationAuctionMinSpacing = 2 days
3. NFT[2] is liquidated, at this time: vaultInfo[auction.nftOwner][auction.auctionAssetContract].count = 0
4. this time, NFT[1] someone to buy, assuming that the price is 50, although still debt 50, but because vaultInfo[auction.nftOwner][auction.auctionAssetContract].count = 0 purchaseLiquidationAuctionNFT() thought it was the last one, so call _reduceDebtWithoutBurn(remaining) to clear the debt 0
5. this time NFT [2] someone to buy, assuming that the price: 50, because the above debt has been cleared 0, so purchaseLiquidationAuctionNFT () will give 50 to alice

This is not reasonable, Alice should not receive 50, because her total debt: 100

    function purchaseLiquidationAuctionNFT(
        Auction calldata auction,
        uint256 maxPrice,
        address sendTo,
        ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo
    ) external override {
        uint256 collateralValueCached = underwritePriceForCollateral(
            auction.auctionAssetContract, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo
        ) * _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count;
        bool isLastCollateral = collateralValueCached == 0;  //***@audit if count==0 ,so collateralValueCached ==0 then isLastCollateral ==true
...

        if (isLastCollateral && remaining != 0) { //***@auit if isLastCollateral and remaining>0,will clear ****//
            /// there will be debt left with no NFTs, set it to 0
            _reduceDebtWithoutBurn(auction.nftOwner, auction.auctionAssetContract, remaining);
        }
    }

Tools Used

Recommended Mitigation Steps

1. The last collateral, debt settlement 0 is not necessary, it can keep the remaining debt

    function purchaseLiquidationAuctionNFT(
        Auction calldata auction,
        uint256 maxPrice,
        address sendTo,
        ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo
    ) external override {
...

-       if (isLastCollateral && remaining != 0) {
-           /// there will be debt left with no NFTs, set it to 0
-            _reduceDebtWithoutBurn(auction.nftOwner, auction.auctionAssetContract, remaining);
-        }    

or If want to clear 0 it is recommended to additionally record how many auction the current user has, not just judge _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L138-L145 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L493-L517

Vulnerability details

Impact

NFT are no longer on the allowed list and can still increase debt

PaprController restricts which NFTs are allowed to be used as collateral by isAllowed. When an NFT is already at risk and no longer suitable as collateral, owner can set isAllowed[nft] to false so that subsequent calls to addCollateral() can not be used as collateral for this NFT, so as to avoid the risk of this NFT is no longer safe.

But here is a problem, the user has been added as collateral in the previous, the user can still continue to borrow through increaseDebt(), and did not check that this NFT is no longer allowed. I think this is unreasonable, if it is no longer in the allowed list, should not be able to borrow through it again. Avoid bringing the risk of funds.

Other:liquidating/repaying is ok, even if NFT is not on the list

Proof of Concept

As follows, increaseDebt() does not check whether the NFT is still on the allowed list

    function _increaseDebt(
        address account,
        ERC721 asset,
        address mintTo,
        uint256 amount,
        ReservoirOracleUnderwriter.OracleInfo memory oracleInfo
    ) internal {
...
    function _increaseDebt(
        address account,
        ERC721 asset,
        address mintTo,
        uint256 amount,
        ReservoirOracleUnderwriter.OracleInfo memory oracleInfo
    ) internal {


    	/***@audit No check  NFT is no longer in the allowed list *****/ 

        uint256 cachedTarget = updateTarget();

        uint256 newDebt = _vaultInfo[account][asset].debt + amount;
        uint256 oraclePrice =
            underwritePriceForCollateral(asset, ReservoirOracleUnderwriter.PriceKind.LOWER, oracleInfo);

        uint256 max = _maxDebt(_vaultInfo[account][asset].count * oraclePrice, cachedTarget);

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

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

        _vaultInfo[account][asset].debt = uint200(newDebt);
        PaprToken(address(papr)).mint(mintTo, amount);

        emit IncreaseDebt(account, asset, amount);
    }

Tools Used

Recommended Mitigation Steps

if NFT don't in allowed list will revert

    function _increaseDebt(
        address account,
        ERC721 asset,
        address mintTo,
        uint256 amount,
        ReservoirOracleUnderwriter.OracleInfo memory oracleInfo
    ) internal {

+       if (!isAllowed[address(asset)]) {
+           revert IPaprController.InvalidCollateral();
+       }

        uint256 cachedTarget = updateTarget();

        uint256 newDebt = _vaultInfo[account][asset].debt + amount;
....                

Lines of code

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

Vulnerability details

Impact

buyAndReduceDebt() may be don't work

The buyAndReduceDebt() method is used to buy PaprToken by uniswap and then use the PaprToken to repay the debt. This method can specify whose debt is to be reduce by specifies the account

However, the internal implementation of the method specifies that the recipient of the uniswap is account, but msg.sender is used to pay off the PaprToken. So if msg.sender and account are not the same person, the PaprToken balance of account will increase, but the balance of msg.sender will be 0. When repayment is made (burnFrom=msg.sender)., the repayment will fail because msg.sender has not received the PaprToken

Proof of Concept

The code 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,    //****@audit account will receive PaprToken ****//
            token0IsUnderlying,
            params.amount,
            params.minOut,
            params.sqrtPriceLimitX96,
            abi.encode(msg.sender)
        );    
...
        //****@audit but burnFrom use msg.sender, msg.sender don't recevie PaprToken ***//
        _reduceDebt({account: account, asset: collateralAsset, burnFrom: msg.sender, amount: amountOut});        

Tools Used

Recommended Mitigation Steps

use msg.sender replace account

    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,
+           msg.sender,
            token0IsUnderlying,
            params.amount,
            params.minOut,
            params.sqrtPriceLimitX96,
            abi.encode(msg.sender)
        );

Lines of code

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

Vulnerability details

Impact

buyAndReduceDebt() when swapFeeBips ! =0 does not work

The buyAndReduceDebt() method is used to buy PaprTokens via uniswap, and then use the purchased PaprTokens to pay off the debt. When swapFeeBips ! = 0, swapFeeTo will be paid some fees

But there is a problem with the implementation, the payment is made using underlying.transfer(params.swapFeeTo) But the user did not transfer underlying to the contract first, the user only willl use underlying.approve(ParaController), so it should be underlying.safeTransferFrom(msg.sender, params.swapFeeTo)

    function buyAndReduceDebt(address account, ERC721 collateralAsset, IPaprController.SwapParams calldata params)
        external
        override
        returns (uint256)
    {
...
        if (hasFee) {
             //***@audit use transfer , should use safeTransferFrom()
            underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);
        }

like uniswap callback use safeTransferFrom() is right buyAndReduceDebt()->uniswap->uniswapV3SwapCallback()

    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata _data) external {
..
        if (isUnderlyingIn) {
            address payer = abi.decode(_data, (address));
            underlying.safeTransferFrom(payer, msg.sender, amountToPay); //***@audit use safeTransferFrom is ok ***//
...

Proof of Concept

BuyAndReduceDebt.t.sol modify the following test , change the fee not to 0 to reproduce this problem

contract BuyAndReduceDebt is BasePaprControllerTest {
    function testBuyAndReduceDebtReducesDebt() public {
...
        IPaprController.VaultInfo memory vaultInfo = controller.vaultInfo(borrower, collateral.addr);
        assertEq(vaultInfo.debt, debt);
        assertEq(underlyingOut, underlying.balanceOf(borrower));
-       uint256 fee;
+       uint256 fee = 1e3;
+       underlying.mint(borrower,underlyingOut * fee / 1e4); //***@audit mint fee to borrow
        underlying.approve(address(controller), underlyingOut + underlyingOut * fee / 1e4);  //***@audit user just approve(),must use safeTransferFrom() in buyAndReduceDebt() ****//
        swapParams = IPaprController.SwapParams({
            amount: underlyingOut,
            minOut: 1,
            sqrtPriceLimitX96: _maxSqrtPriceLimit({sellingPAPR: false}),
            swapFeeTo: address(5),
            swapFeeBips: fee
        });
        uint256 debtPaid = controller.buyAndReduceDebt(borrower, collateral.addr, swapParams);            

Tools Used

Recommended Mitigation Steps

use safeTransferFrom()

    function buyAndReduceDebt(address account, ERC721 collateralAsset, IPaprController.SwapParams calldata params)
        external
        override
        returns (uint256)
    {
....

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

PaprController.sol liquidationsLocked only prevents liquidation, it does not prevent borrowing，Suggest adding a pause switch to stop borrowing and avoid security risks

contract PaprController is
+    bool paused;


    function _increaseDebt(
        address account,
        ERC721 asset,
        address mintTo,
        uint256 amount,
        ReservoirOracleUnderwriter.OracleInfo memory oracleInfo
    ) internal {
+    require(!paused);
...

+  function setPaused(bool _paused) external onlyOwner{
+      paused = _paused;
+   }