Contract owner can drain all NFT collateral

[code423n4]

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L365-L379 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L456-L479 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L414-L416

Vulnerability details

Impact

To add NFTs as collateral to the protocol and it to create debt, the owner has to approve each NFT contract address first.

As anyone can create their own papr vaults with arbitrary allowed NFTs by design, this opens the possibility for malicious actors to deploy a benevolent looking vault first (e.g. with PUNKs as collateral) and later allow a price manipulated NFT to mint papr and dump it on the market. This will inflate funding rates and target, which can then be exploitet to liquidate the valuable NFTs.

Proof of Concept

function setAllowedCollateral(IPaprController.CollateralAllowedConfig[] calldata collateralConfigs)
    external
    override
    onlyOwner
{
    for (uint256 i = 0; i < collateralConfigs.length;) {
        if (collateralConfigs[i].collateral == address(0)) revert IPaprController.InvalidCollateral();

        isAllowed[collateralConfigs[i].collateral] = collateralConfigs[i].allowed;
        emit AllowCollateral(collateralConfigs[i].collateral, collateralConfigs[i].allowed);
        unchecked {
            ++i;
        }
    }
}

The owner can allow arbitrary NFT as collateral. E.g. add PUNKs first. Then after people added some PUNKs and minted papr allow a custom NFT collection where you manipulate the floor price (As long as there is no restriction on the ReservoirOracle, any custom collection can be created).

function _increaseDebt(
    address account,
    ERC721 asset,
    address mintTo,
    uint256 amount,
    ReservoirOracleUnderwriter.OracleInfo memory oracleInfo
) internal {
    uint256 cachedTarget = updateTarget();

    uint256 newDebt = _vaultInfo[account][asset].debt + amount;
    uint256 oraclePrice =
        underwritePriceForCollateral(asset, ReservoirOracleUnderwriter.PriceKind.LOWER, oracleInfo);

    uint256 max = _maxDebt(_vaultInfo[account][asset].count * oraclePrice, cachedTarget); // MaxDebt is done for each NFT contract seperately

    if (newDebt > max) revert IPaprController.ExceedsMaxDebt(newDebt, max);

    if (newDebt >= 1 << 200) revert IPaprController.DebtAmountExceedsUint200();

    _vaultInfo[account][asset].debt = uint200(newDebt);
    PaprToken(address(papr)).mint(mintTo, amount); // But papr minting isn't done for each NFT contract seperately!

    emit IncreaseDebt(account, asset, amount);
}

Because debt is done on a per NFT basis, but papr is the same for all NFT contracts in the vault, we can use the papr from the malicious NFT collection to get valuable papr from the valuable collections.

Recommended Mitigation Steps

Either put a restriction on ReservoirOracle to only permit specific NFT collections (Not recommended, since someone could still deploy their own PaprController with custom oracleSigner) or do papr minting for each NFT in a vault seperately.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid