Lines of code

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

Vulnerability details

Impact

Unintended leniency of protocol will be taken advantage by users. Users can withdraw NFTs even if their debt of a particular NFT is higher than intended.

Proof of Concept

When a user wants to removeCollateral(), he calls removeCollateral which loops each collateral that he has and calls _removeCollateral on each one of them.

function removeCollateral(
    address sendTo,
    IPaprController.Collateral[] calldata collateralArr,
    ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo
) external override {
    uint256 cachedTarget = updateTarget();
    uint256 oraclePrice;
    ERC721 collateralAddr;

    for (uint256 i = 0; i < collateralArr.length;) {
        if (i == 0) {
            collateralAddr = collateralArr[i].addr;
            oraclePrice =
                underwritePriceForCollateral(collateralAddr, ReservoirOracleUnderwriter.PriceKind.LOWER, oracleInfo);
        } else {
            if (collateralAddr != collateralArr[i].addr) {
                revert CollateralAddressesDoNotMatch();
            }
        }

        _removeCollateral(sendTo, collateralArr[i], oraclePrice, cachedTarget);

However, the oraclePrice is only calculated once before passing the value into _removeCollateral. The subsequent NFTs will be following the oraclePrice. If the first NFT is much more expensive than the subsequent NFTs in the collateralArr, then the maxDebt will always be higher. As such, the user can take advantage of the protocol's unintended leniency and withdraw most of his lower priced NFTs. In _removeCollateral, if debt is higher than max, then function will revert. Because maxDebt is now much greater since the oraclePrice will always be matched against the first in the loop, debt will not likely be higher than max. The user can withdraw the rest of his NFTs without worry.

    collateral.addr.safeTransferFrom(address(this), sendTo, collateral.id);

    uint256 debt = _vaultInfo[msg.sender][collateral.addr].debt;
    uint256 max = _maxDebt(oraclePrice * newCount, cachedTarget);

    if (debt > max) {
        revert IPaprController.ExceedsMaxDebt(debt, max);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Check the OraclePrice of each NFT.

code-423n4 / 2022-12-backed-findings