Papr contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

PaprController#buyAndReduceDebt double charges msg.sender if msg.sender != account. This leads to either loss of funds or the inability to reduce debt for other users

Proof of Concept

(uint256 amountOut, uint256 amountIn) = UniswapHelpers.swap(
    pool,
    account, //@audit account is the recipient of the swap
    token0IsUnderlying,
    params.amount,
    params.minOut,
    params.sqrtPriceLimitX96,
    abi.encode(msg.sender) //@audit msg.sender is payer
);

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

//@audit papr is burned from msg.sender even though the papr was sent to account
_reduceDebt({account: account, asset: collateralAsset, burnFrom: msg.sender, amount: amountOut});

UniswapHelpers.swap sends the papr obtained from the swap to account. Later when reducing the debt of account it burns papr from msg.sender rather than account. This has two outcomes when a user is paying off debt for another user. The transaction will either revert because msg.sender doesn't have enough papr to burn or msg.sender will be double charged. First they will be charge underlying when swapping then they will be charged again when the papr is burned from them in the _reduceDebt subcall.

Tools Used

Manual Review

Recommended Mitigation Steps

The swap output should be sent to msg.sender rather than account:

    (uint256 amountOut, uint256 amountIn) = UniswapHelpers.swap(
        pool,
-       account,
+       msg.sender,
        token0IsUnderlying,
        params.amount,
        params.minOut,
        params.sqrtPriceLimitX96,
        abi.encode(msg.sender)
    );

Lines of code

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

Vulnerability details

Impact

PaprController#buyAndReduceDebt will either revert or steal underlying from contract when the swap has a fee

Proof of Concept

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

If there is a swap fee for PaprController#buyAndReduceDebt it will attempt to transfer the fee from the contract. This leads to one of two issues:

1. If paprController doesn't have any enough underlying then the call will always revert breaking the function
2. If it has enough then it will steal underlying from the contract by forcing the contract to pay the swap fees for the user

Tools Used

Manual Review

Recommended Mitigation Steps

PaprController already handles the underlying transfer for the swap via uniswapV3SwapCallback which means the user has already approved PaprController to transfer underlying. Use safeTransferFrom instead:

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

Lines of code

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

Vulnerability details

Impact

Users are unable to withdraw there collateral without an oracle signature even if they have no debt

Proof of Concept

Under every circumstance in which a user removes collateral from the contract they must provide an oracle signed price for the NFT. This includes users with no debt. Also the oracle is immutable so it can't be replaced. In the event that the oracle become unrecoverable, all user collateral will become permanently stuck in the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Bypass the oracle check if a user doesn't have any debt and is removing their collateral. Since the user carries no debt, there is no reason to check the price