function buyAndReduceDebt() spend more underlying token than user specified and also code doesn't check that swapFeeBips is less than BIPS_ONE and user can lose some of his underlying token balance that he gave protocol spending approval

[code423n4]

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L182-L205
https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L208-L232
https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L493-L517

Vulnerability details

Impact

user can specify fee recipient and fee amount to send to that recipient and it is calculated by amount * swapFeeBips / BIPS_ONE but there is no check in the code to make sure swapFeeBips is less than BIPS_ONE and if user set wrong value by mistake or client set malicious values then user would lose his received underlying tokens in the transaction or even he can lose his underlying balance up to spending approval amount of contract from user (in function buyAndReduceDebt() code should transfer fee from user address and it can transfer up to approval amount).

Proof of Concept

This is buyAndReduceDebt() code:

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

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

        return amountOut;
    }

as you can see in line underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE); code transfers fee (this line has a bug and it should have transferred fee from msg.sender) and the amount of the fee is calculated by user specified parameters (which clients can manipulate them) and there is no check for the maximum fee amount and if wrong amounts set for the fee then user can lose a lot of underlying asset (up to underlying.approval[user][PaprContract]). functions increaseDebtAndSell() and _increaseDebtAndSell() don't have checks for swapFeeBips and if the value was too high user can lose all the underlying balance of he should have received (100% of the amount can go to fee recipient) but in these function user can lose up to the received amount from loan swap. the attack steps is like this:

1. user give PaprControl contract uint256.max spending approval for underlying token so contract can spend all user underlying balance.
2. user wants to pay some of his debts by his underlying tokens so he calls buyAndReduceDebt().
3. user makes a mistake and sets the swapFeeBips very high even higher than BIPS_ONE (or buggy client make a mistake and set the value very high wrongly or client do it by intention).
4. the call would repay user debt but when code wants to pay the fee to fee recipient it would transfer a lot of user funds because swapFeeBips is bigger than BIPS_ONE and user could lose all of his underlying balance.

so contract should have protected user by checking the value of the swapFeeBips. checking swapFeeBips < BIPS_ONE is critical because never ever the should be bigger than the amount user spending and having a max value for fee is reassuring too and can prevent fund loss because of the mistakes or bad clients.

There is another bug here which is contract spends more funds of user that what user specified. in the function increaseDebtAndSell() and _increaseDebtAndSell() contract gets fee from amount that is in contract so spending_amount = allowed_amount = fee + user_received_amount

        if (hasFee) {
            uint256 fee = amountOut * params.swapFeeBips / BIPS_ONE;
            underlying.transfer(params.swapFeeTo, fee);
            underlying.transfer(proceedsTo, amountOut - fee);
        }

but in the function buyAndReduceDebt() contract gets fee from user address so spending_amount = allowed_amount + fee = user_received_amount + fee, so contract could spend more amount of underlying token than what user specified.

        if (hasFee) {
            underlying.transferFrom(msg.sender, params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);  // fixed the bug and send from msg.ssender
        }

This is a important issue because user can lose all of his underlying balance and also contract is always using more funds than user specified in buyAndReduceDebt()

Tools Used

VIM

Recommended Mitigation Steps

spend what user specified and also check the swapFee to make sure user won't lose his funds.

[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