buyAndReduceDebt() function will revert while params.swapFeeBips != 0

[code423n4]

Lines of code

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

Vulnerability details

Impact

The buyAndReduceDebt() function wrongly charges swap fee from PaprController contract itsself rather than the msg.sender. As normally the PaprController contract never holds any underlying asset, so the call to buyAndReduceDebt() will always revert while params.swapFeeBips != 0.

Proof of Concept

Code and audit comments related to the vulnerability

    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); // @audit should transfer from msg.sender
        }

        _reduceDebt({account: account, asset: collateralAsset, burnFrom: msg.sender, amount: amountOut});

        return amountOut;
    }

And the test case, put it into BuyAndReduceDebt contract of test\paprController\BuyAndReduceDebt.t.sol

    function testBuyAndReduceDebtWithFeeRevert() 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 halfToRepay = underlyingOut / 2;
        uint256 fee = 500;
        assertGt(underlying.balanceOf(borrower), halfToRepay + halfToRepay * fee / 1e4);
        underlying.approve(address(controller), halfToRepay + halfToRepay * fee / 1e4);
        swapParams = IPaprController.SwapParams({
            amount: underlyingOut,
            minOut: 1,
            sqrtPriceLimitX96: _maxSqrtPriceLimit({sellingPAPR: false}),
            swapFeeTo: address(5),
            swapFeeBips: fee
        });
        vm.expectRevert();
        controller.buyAndReduceDebt(borrower, collateral.addr, swapParams);
    }

Run forge test --match-test testBuyAndReduceDebtWithFeeRevert, and the result

Running 1 test for test/paprController/BuyAndReduceDebt.t.sol:BuyAndReduceDebt
[PASS] testBuyAndReduceDebtWithFeeRevert() (gas: 430158)
Test result: ok. 1 passed; 0 failed; finished in 633.04ms

Tools Used

foundry

Recommended Mitigation Steps

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

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

    // ...
}

[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