Papr contest - Franfran's results

Lines of code

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

Vulnerability details

Impact

When a user is buying the debt of an account (or its own debt), it either have the choice to use the reduceDebt() function in order to pay with PAPR tokens, or by paying in underlying tokens (can be USDC, WETH, ...). A protocol that would choose to permissionlessly build on top of Papr could incentivize his users to pay some kind of extra fee to the Papr protocol. In the case of a protocol doing this in order to reduce a user debt, that won't work. The reason of that is because of an underflow that is reverting the transaction when the fees are non-0. In the current state, the contract is trying to transfer the underlying tokens from the PaprController to the fees receiver, but it should extract them from the transaction sender in order to pay the extra to this receiver.

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

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

Proof of Concept

Please find the following test showing that a non-zero fee will occur in an underflow that can be plugged in the BuyAndReduceDebt.t.sol

    function testWithFeesReverts() 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 fee = 1; // smol fee
        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);
    }

Result:

Failing tests:
Encountered 1 failing test in test/paprController/BuyAndReduceDebt.t.sol:BuyAndReduceDebt
[FAIL. Reason: Arithmetic over/underflow] testWithFeesReverts() (gas: 449122)

As you can see, a fee of 1 wei is sufficient to make the test fail because the PaprController does not have any underlying tokens.

Tools Used

Manual inspection

Recommended Mitigation Steps

Change the following snippet in order to transfer the fees from the user to the receiver rather than extracting them from the PaprController.

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

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

[Low-1] oracleSigner is immutable

If the keys of the oracleSigner are lost, then the market will be frozen as the signed message requires to be "fresh" (signed at a timestamp that does not exceed 20 minutes). It's a good feature for transparency to have oracleSigner as an immutable variable, but in some cases it can be really dangerous. There is no need to recall past hacks due to bad OPSEC, but losing the private key, sharing it by error (phishing, hacking ...) had some pretty bad consequences.

A solution would be to make it a storage variable and being able to change it from a multisig and a timelock to mitigate those types of issues.

[Low-2] The oracle can be the victim of a signature replay attack

underwritePriceForCollateral() is not EIP712 compliant. It should pass the DOMAIN_SEPARATOR() in order to avoid this issue. This is written in the EIP-712 specification:

"The way to solve this (replay attacks) is by introducing a domain separator, a 256-bit number. This is a value unique to each domain that is ‘mixed in’ the signature. It makes signatures from different domains incompatible. The domain separator is designed to include bits of DApp unique information such as the name of the DApp, the intended validator contract address, the expected DApp domain name, etc. The user and user-agent can use this information to mitigate phishing attacks, where a malicious DApp tries to trick the user into signing a message for another DApp."

https://eips.ethereum.org/EIPS/eip-712

As from the whitepaper, the oracle is pushing the current prices using signed messages and took "TrustUs" as a reference point which included this domain separator.

[NC-1] Incorrect error in underwritePriceForCollateral()

if (
    oracleInfo.message.timestamp > block.timestamp || oracleInfo.message.timestamp + VALID_FOR < block.timestamp
) {
    revert OracleMessageTooOld();
}

The error that is returned when the price from the oracle is too "young" or too "old" seems to be incorrect. A quick fix would be to change it to:

if (oracleInfo.message.timestamp > block.timestamp) {
    revert OracleMessageTooYoung();
} else if (oracleInfo.message.timestamp + VALID_FOR < block.timestamp) {
    revert OracleMessageTooOld();
}

[NC-2] Misleading information in the NATSPEC comment

This one is related to my other finding "There is no way to extract fees when someones want to reduce a debt by paying with underlying tokens". In the IPaprController, it is explained that the buyAndReduceDebt removes debt from a vault while allowing to pay with the controllers underlying tokens, while the tokens should clearly go from the payer to the Uniswap pool in order to buy some PAPR. Invalid concern if the meaning was that the underlying were concerning the address of the underlying that is accepted to be used in order to repay the debt of a vault.

[NC-3] Typo

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/UniswapOracleFundingRateController.sol#L150 _lastUpdates should be replaced with _lastUpdated.