Papr contest - Saintcode_'s results

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L225-L227 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/BuyAndReduceDebt.t.sol#L12-L43

Vulnerability details

Impact

When the User specifies a swap fee swapFeeBips different from 0, the function buyAndReduceDebt always reverts leaving the swap fee feature useless [DOS].

Furthermore, the current implementation gives the user the ability to transfer any amount of the underlying balance of PaprCopntroller to the swapFeeTo specified, even though the controller is not supposed to hold any underlying balance is important to fix this issue for future features where it could hold or hidden possible states.

I ranked this vulnerability as medium risk because of the DOS impact but, the user being able to transfer any amount of the underlying balance of PaprCopntroller if it somehow would hold any underlying, even though is not very probable, in my opinion makes it eligible for high risk.

Proof of Concept

Reusing testBuyAndReduceDebtReducesDebtWithFee() test: https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/BuyAndReduceDebt.t.sol#L12-L43

I have written the following PoC test initializing the fee variable to a value different from 0, to prove that the execution will always revert:

    function testBuyAndReduceDebtReducesDebtWithFee() 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));

        //FEE VALUE INITIALIZED TO AN INTEGER DIFFERENT TO 0
        uint256 fee = 2;

        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);
    }

This PoC test always reverts.

Based on the tests on the BuyAndReduceDebt.t.sol file: https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/BuyAndReduceDebt.t.sol

My interpretation of the desired functionality of the swap fee is to be transferred directly from the user to the fee receiver. Because on both tests, before initializing the SwapParams object to call buyAndReduce(), the controller is approved the (SwapAmount plus the FeeAmount): underlying.approve(address(controller), underlyingOut + underlyingOut * fee / 1e4); .

As the fee variable was not initialized on the tests, this scenario was not being tested.

Therefore the transfer() function on line 226 of PaprController.sol. https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L226

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

Should be changed to a transferFrom function with msg.sender as the from parameter in order to make this functionality work and prevent future exploits if any time the controller holds underlying balance:

underlying.transferFrom(msg.sender, params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);

Tools Used

Foundry, Visual Studio

Recommended Mitigation Steps

As said earlier the transfer function should be changed to a transferFrom function with msg.sender as first parameter.