'buyAndReduceDebt()' will always revert if called with a fee parameter set

[code423n4]

Lines of code

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

Vulnerability details

Impact

There is a bug in PaprController.buyAndReducedebt() in which a caller-set fee is incorrectly transfered via transfer() to the fee receiver. Because the PaprController.sol won't be holding any underlying tokens this transaction will always revert when the fee is set. The fee should be paid by the msg.sender and not by the protocol.

/// @inheritdoc IPaprController
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;
}

We can see above that if hasFee == true , which is inputted by the caller, amountIn * params.swapFeeBips / BIPS_ONE amount of underlying tokens are transferred to a fee receiver, also inputted by the caller.

This fee is sent out to params.swapfeeTo:
underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);
Because the PaprController.sol contract won't be holding any underlying token, this transfer would always revert the whole transaction with Arithmetic underflow since the ERC20.transfer() function is trying to substract the fee amount from 0:

function transfer(address to, uint256 amount) public virtual returns (bool) {
    balanceOf[msg.sender] -= amount;
    // Cannot overflow because the sum of all user
    // balances can't exceed the max uint256 value.
    unchecked {
        balanceOf[to] += amount;
    }
    emit Transfer(msg.sender, to, amount);
    return true;
}

If PaprController.sol would ever be holding the underlying token it would be possible to drain it immediately but because this is not the case and user funds are not at direct risk I will report this as Medium.

Proof of Concept

Modifying the test/paprController/BuyAndReduceDebt.t.sol contract function testBuyAndReduceDebtReducesDebt() so that the fee is something more than zero will point out the issue.

function testBuyAndReduceDebtReducesDebt() public {
    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);
    IPaprController.VaultInfo memory vaultInfo = controller.vaultInfo(borrower, collateral.addr);
    assertEq(vaultInfo.debt, debt);
    assertEq(underlyingOut, underlying.balanceOf(borrower));
    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
    });
    uint256 debtPaid = controller.buyAndReduceDebt(borrower, collateral.addr, swapParams);
    assertGt(debtPaid, 0);
    vaultInfo = controller.vaultInfo(borrower, collateral.addr);
    assertEq(vaultInfo.debt, debt - debtPaid);
    assertEq(underlying.balanceOf(swapParams.swapFeeTo), fee);
}

Bob wants to call PaprController.buyAndReduceDebt() function with a fee sent to a specific address but because of the issue at hand the transaction would revert.

Tools Used

Manual review with VS Code.

Recommended Mitigation Steps

Use the safeTransferFrom() function from SafeTransferLib.sol instead of transfer(). This way the fees will be transferred from msg.sender to swapFeesTo and would revert on failure.

[trust1995]

Seems like non-core functionality may be impaired. Will let sponsor weigh in.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as primary issue

[wilsoncusack]

Yup. Big miss on our part

[c4-sponsor]

wilsoncusack marked the issue as sponsor confirmed

[C4-Staff]

JeeberC4 marked the issue as duplicate of #196