Papr contest - rvierdiiev's results

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L264-L294 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L532-L552

Vulnerability details

Impact

Because purchaseLiquidationAuctionNFT function clears remaining debt of debtor if he has no more collateral, it's possible that when 2 auctions exists in same time, liquidation logic will not work properly and debt will be nullified before last auction is finished.

Proof of Concept

When user has a bad debt, then anyone can start auction for his nft using startLiquidationAuction function. Also there is check that 2 days have passed since last auction for the debtor was created. https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L321-L323

        if (block.timestamp - info.latestAuctionStartTime < liquidationAuctionMinSpacing) {
            revert IPaprController.MinAuctionSpacing();
        }

The starting price for auction is 3*oraclePrice in papr. That means that usually noone will buy token at start time, they will be waiting when price will decrease. So it's actually possible that after 2 days the first auction still not finished and someone will start new one if debtor has another collateral token in vault.

To purchase token, liquidator can call purchaseLiquidationAuctionNFT function. https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L264-L294

    function purchaseLiquidationAuctionNFT(
        Auction calldata auction,
        uint256 maxPrice,
        address sendTo,
        ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo
    ) external override {
        uint256 collateralValueCached = underwritePriceForCollateral(
            auction.auctionAssetContract, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo
        ) * _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count;
        bool isLastCollateral = collateralValueCached == 0;


        uint256 debtCached = _vaultInfo[auction.nftOwner][auction.auctionAssetContract].debt;
        uint256 maxDebtCached = isLastCollateral ? debtCached : _maxDebt(collateralValueCached, updateTarget());
        /// anything above what is needed to bring this vault under maxDebt is considered excess
        uint256 neededToSaveVault = maxDebtCached > debtCached ? 0 : debtCached - maxDebtCached;
        uint256 price = _purchaseNFTAndUpdateVaultIfNeeded(auction, maxPrice, sendTo);
        uint256 excess = price > neededToSaveVault ? price - neededToSaveVault : 0;
        uint256 remaining;


        if (excess > 0) {
            remaining = _handleExcess(excess, neededToSaveVault, debtCached, auction);
        } else {
            _reduceDebt(auction.nftOwner, auction.auctionAssetContract, address(this), price);
            remaining = debtCached - price;
        }


        if (isLastCollateral && remaining != 0) {
            /// there will be debt left with no NFTs, set it to 0
            _reduceDebtWithoutBurn(auction.nftOwner, auction.auctionAssetContract, remaining);
        }
    }

What is interesting for us is this check bool isLastCollateral = collateralValueCached == 0;. Using this check protocol checks if debtor still has any collateral. It is needed to nullify debt in case if it's still remaining and there is no collateral anymore.

The problem here is that protocol doesn't handle situation when few auctions are in progress is same time.

Problem scenario. 1.Debtor has 2 nft as collateral and he has bad debt. 2.startLiquidationAuction is called for 1 nft. 3.after 2 days no one still bought that nft and startLiquidationAuction is called for 2 nft. 4.Now debtor doesn't have any collateral. Debtor calls purchaseLiquidationAuctionNFT for 1 nft(it's cheap now). isLastCollateral param will be 0 and that means that the debt will be cleared after the token purchase. 5.Debtor calls purchaseLiquidationAuctionNFT for 2 nft and because his debt is 0(was nullified), then full token price(minus fees) will be sent to him as excess value. 6.As result debtor received 2 nft back and also full token price for 1 nft. Protocol didn't receive debt repayment.

As you can see at a time when no more collateral tokens is left in debtor's vault then the first purchase of any auctioned token will fully clear the debt which will benefit debtor, and leaves protocol with unpaid debts.

Tools Used

VsCode

Recommended Mitigation Steps

In case if debtor has more auctions then do not clear his debt if he has no more collateral tokens.

Lines of code

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

Vulnerability details

Impact

Reentrancy attack allows to get loan for free when startLiquidationAuction is called on last collateral token.

Proof of Concept

When user has a bad debt, then anyone can start auction for his nft. To purchase token, liquidator can call purchaseLiquidationAuctionNFT function. https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L264-L294

    function purchaseLiquidationAuctionNFT(
        Auction calldata auction,
        uint256 maxPrice,
        address sendTo,
        ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo
    ) external override {
        uint256 collateralValueCached = underwritePriceForCollateral(
            auction.auctionAssetContract, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo
        ) * _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count;
        bool isLastCollateral = collateralValueCached == 0;


        uint256 debtCached = _vaultInfo[auction.nftOwner][auction.auctionAssetContract].debt;
        uint256 maxDebtCached = isLastCollateral ? debtCached : _maxDebt(collateralValueCached, updateTarget());
        /// anything above what is needed to bring this vault under maxDebt is considered excess
        uint256 neededToSaveVault = maxDebtCached > debtCached ? 0 : debtCached - maxDebtCached;
        uint256 price = _purchaseNFTAndUpdateVaultIfNeeded(auction, maxPrice, sendTo);
        uint256 excess = price > neededToSaveVault ? price - neededToSaveVault : 0;
        uint256 remaining;


        if (excess > 0) {
            remaining = _handleExcess(excess, neededToSaveVault, debtCached, auction);
        } else {
            _reduceDebt(auction.nftOwner, auction.auctionAssetContract, address(this), price);
            remaining = debtCached - price;
        }


        if (isLastCollateral && remaining != 0) {
            /// there will be debt left with no NFTs, set it to 0
            _reduceDebtWithoutBurn(auction.nftOwner, auction.auctionAssetContract, remaining);
        }
    }

What is interesting for us is this check bool isLastCollateral = collateralValueCached == 0;. Using this check protocol checks if debtor still has any collateral. It is needed to nullify debt in case if it's still remaining and there is no collateral anymore. This check is done in the top of function. Later _purchaseNFTAndUpdateVaultIfNeeded function is called which will allow attacker to get a hook in his contract.

Now inside this hook attacker can call PaprController.addCollateral to add some amount of nft and later call PaprController.increaseDebt to get a loan. Let's imagine that after this manipulation the debt of attacker will be 1 million. At this moment attacker is done and execution is returned into purchaseLiquidationAuctionNFT function where we have isLastCollateral function as true. That means that in the end of function the debt of 1 million will be set to 0. And after that attacker will be able to remove his collateral.

So this is full attack scenario. 1.Attacker has 1 nft with bad debt. 2.He starts auction for it. 3.He calls purchaseLiquidationAuctionNFT function. 4.When purchased nft is sent to attacker then hook is called. 5.The hook adds as much as possible in collateral and takes maximum loan. 6.Execution is returned back to purchaseLiquidationAuctionNFT function which clears attackers debt. 7.Attacker removes his collateral.

Tools Used

VsCode

Recommended Mitigation Steps

Transfer _purchaseNFTAndUpdateVaultIfNeeded function to the bottom of function to make all state variables changes before transferring token.

Lines of code

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

Vulnerability details

Impact

PaprController.buyAndReduceDebt sends fee from PaprController's funds instead of user's. Because PaprController usually doesn't hold funds the function will always fail when fee is set.

Proof of Concept

PaprController.buyAndReduceDebt buys papr token from uniswap pool using underlying tokens. If fee should be sent, then it sends them to fee recipient. But instead of sending them from user's account it uses funds of PaprController. https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L226

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

The problem here is that it uses transfer function instead of transferFrom. As result, because PaprController should not control any funds, this function will always revert, when fee should be sent.

Tools Used

VsCode

Recommended Mitigation Steps

Use transferFrom to send fee from user's account.

Lines of code

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

Vulnerability details

Impact

Because there is no checking of input params in constructor of PaprController it's possible to initialize it with incorrect values.

Proof of Concept

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

    constructor(
        string memory name,
        string memory symbol,
        uint256 _maxLTV,
        uint256 indexMarkRatioMax,
        uint256 indexMarkRatioMin,
        ERC20 underlying,
        address oracleSigner
    )
        NFTEDAStarterIncentive(1e17)
        UniswapOracleFundingRateController(underlying, new PaprToken(name, symbol), indexMarkRatioMax, indexMarkRatioMin)
        ReservoirOracleUnderwriter(oracleSigner, address(underlying))
    {
        maxLTV = _maxLTV;
        token0IsUnderlying = address(underlying) < address(papr);
        uint256 underlyingONE = 10 ** underlying.decimals();
        uint160 initSqrtRatio;


        // initialize the pool at 1:1
        if (token0IsUnderlying) {
            initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(underlyingONE, 10 ** 18);
        } else {
            initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(10 ** 18, underlyingONE);
        }


        address _pool = UniswapHelpers.deployAndInitPool(address(underlying), address(papr), 10000, initSqrtRatio);


        _init(underlyingONE, _pool);
    }

Constructor of PaprController doesn't check input params and set them as they are provided. Such approach can create problems. For example if oracleSigner param is provided as 0 address, that means that it will be possible to provide corrupted signed messages with incorrect values and pass check for oracle signer.

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/ReservoirOracleUnderwriter.sol#L68-L90

        address signerAddress = ecrecover(
            keccak256(
                abi.encodePacked(
                    "\x19Ethereum Signed Message:\n32",
                    // EIP-712 structured-data hash
                    keccak256(
                        abi.encode(
                            keccak256("Message(bytes32 id,bytes payload,uint256 timestamp)"),
                            oracleInfo.message.id,
                            keccak256(oracleInfo.message.payload),
                            oracleInfo.message.timestamp
                        )
                    )
                )
            ),
            oracleInfo.sig.v,
            oracleInfo.sig.r,
            oracleInfo.sig.s
        );


        if (signerAddress != oracleSigner) {
            revert IncorrectOracleSigner();
        }

As you can see in case if ecrecover returns 0(when signature is corrupted) and oracleSigner is 0 then it's possible to provide as big price for your nft as you wish and steal protocol funds.

Another check that should be added is that indexMarkRatioMax > indexMarkRatioMin. Also that maxLTV is in some correct borders.

Tools Used

VsCode

Recommended Mitigation Steps

Add input validation in PaprController constructor.