Faulty fee handling in `buyAndReduceDebt`

[code423n4]

Lines of code

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

Vulnerability details

Impact

When passing fee params to buyAndReduceDebt with swapFeeTo and swapFeeBips, the PaprController will try to send the underlying token on the following line:

underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);

But in a normal state, the controller won't have any underlying token as they are sent directly to the Uniswap pool. Therefore the function will fail if a fee is passed.

And if the controller were to hold these tokens (usdc for instance) this could be even worse as an attacker could drain them by buying small amounts of Papr token passing a big fee (bigger than the amountIn - there is no check on that).

Proof of Concept

It only takes modifying the test testBuyAndReduceDebtReducesDebt :

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

resulting in the following error:

Encountered 1 failing test in test/paprController/BuyAndReduceDebt.t.sol:BuyAndReduceDebt
[FAIL. Reason: Arithmetic over/underflow] testBuyAndReduceDebtReducesDebt() (gas: 454860)

happening on the the line of the underlying token transfer.

Tools Used

forge

Recommended Mitigation Steps

The same way it's done in increaseDebtAndSell to get the underlying token first on the controller and then send them to the fee recipient and the user

[c4-judge]

trust1995 marked the issue as duplicate of #20

[c4-judge]

trust1995 marked the issue as satisfactory

[C4-Staff]

JeeberC4 marked the issue as duplicate of #196