Calling `ParticleExchange.sellNftToMarket`, `ParticleExchange.swapWithEth`, and `ParticleExchange.refinanceLoan` functions can allow insolvent positions to be opened

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L253-L289 https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L434-L463 https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L544-L613 https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L192-L223

Vulnerability details

Impact

When calling the following ParticleExchange.sellNftToMarket function, it is possible to input amount and msg.value, which can sum up to lien.price and make credit equal 0. Although credit of liens[lienId] is 0 in this case, calling the ParticleExchange.sellNftToMarket function by the borrower does not revert.

https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L253-L289

    function sellNftToMarket(
        Lien calldata lien,
        uint256 lienId,
        uint256 amount,
        bool pushBased,
        address marketplace,
        bytes calldata tradeData
    ) external payable override validateLien(lien, lienId) {
        ...

        /// @dev cannot instantly become insolvent (will revert if credit < 0)
        uint256 credit = amount + msg.value - lien.price;

        // update lien
        liens[lienId] = keccak256(
            abi.encode(
                Lien({
                    lender: lien.lender,
                    borrower: msg.sender,
                    collection: lien.collection,
                    tokenId: lien.tokenId,
                    price: lien.price,
                    rate: lien.rate,
                    loanStartTime: block.timestamp,
                    credit: credit,
                    auctionStartTime: 0
                })
            )
        );

        ...
    }

Similarly, it is also possible to call the following ParticleExchange.swapWithEth function with msg.value equaling lien.price. Although this causes credit of liens[lienId] to be 0, calling the ParticleExchange.swapWithEth function does not revert either.

https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L434-L463

    function swapWithEth(Lien calldata lien, uint256 lienId) external payable override validateLien(lien, lienId) {
        ...

        /// @dev cannot instantly become insolvent (will revert if credit < 0)
        uint256 credit = msg.value - lien.price;

        // update lien
        liens[lienId] = keccak256(
            abi.encode(
                Lien({
                    lender: lien.lender,
                    borrower: msg.sender,
                    collection: lien.collection,
                    tokenId: lien.tokenId,
                    price: lien.price,
                    rate: lien.rate,
                    loanStartTime: block.timestamp,
                    credit: credit,
                    auctionStartTime: 0
                })
            )
        );

        ...
    }

Moreover, uint256 newCredit = (oldLien.credit + oldLien.price - payableInterest) - newLien.price can possibly be evaluated to 0 in the following ParticleExchange.refinanceLoan function. Although credit of liens[newLienId] is 0 in this situation, calling the ParticleExchange.refinanceLoan function does not revert.

https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L544-L613

    function refinanceLoan(
        Lien calldata oldLien,
        uint256 oldLienId,
        Lien calldata newLien,
        uint256 newLienId
    ) external override validateLien(oldLien, oldLienId) validateLien(newLien, newLienId) {
        ...
        uint256 newCredit = (oldLien.credit + oldLien.price - payableInterest) - newLien.price;

        ...

        // update new lien
        liens[newLienId] = keccak256(
            abi.encode(
                Lien({
                    lender: newLien.lender,
                    borrower: oldLien.borrower,
                    collection: newLien.collection,
                    tokenId: newLien.tokenId,
                    price: newLien.price,
                    rate: newLien.rate,
                    loanStartTime: block.timestamp,
                    credit: newCredit,
                    auctionStartTime: 0
                })
            )
        );

        ...
    }

However, when lien.credit is 0, the following ParticleExchange.withdrawEthWithInterest function would evaluate payableInterest < lien.credit to false and consider the borrower insolvent. This means when the respective lien's credit is evaluated to 0 when the ParticleExchange.sellNftToMarket, ParticleExchange.swapWithEth, and ParticleExchange.refinanceLoan functions are called, the lender of the respective lien can immediately call the ParticleExchange.withdrawEthWithInterest function, which would cause the respective lien's borrower to unexpectedly lose its position margin without having a chance to buy an NFT for repaying the lien.

https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L192-L223

    function withdrawEthWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
        ...

        uint256 payableInterest = _calculateCurrentPayableInterest(lien);

        // verify that the liquidation condition has met (borrower insolvent or auction concluded)
        if (payableInterest < lien.credit && !_auctionConcluded(lien.auctionStartTime)) {
            revert Errors.LiquidationHasNotReached();
        }

        // delete lien (delete first to prevent reentrancy)
        delete liens[lienId];

        // transfer ETH with interest back to lender
        payable(lien.lender).transfer(lien.price + payableInterest);

        // transfer PnL to borrower
        if (lien.credit > payableInterest) {
            payable(lien.borrower).transfer(lien.credit - payableInterest);
        }

        emit WithdrawETH(lienId);

        // withdraw interest from this account too
        _withdrawAccountInterest(payable(msg.sender));
    }

Proof of Concept

The following steps can occur for the described scenario for the ParticleExchange.sellNftToMarket function. The cases for the ParticleExchange.swapWithEth and ParticleExchange.refinanceLoan functions are similar.

1. Alice calls the ParticleExchange.sellNftToMarket function with amount and msg.value both being 0.5e18 while lien.price is 1e18, which causes the lien's credit to be 0.
2. Bob, who is the lien's lender, immediately calls the ParticleExchange.withdrawEthWithInterest function to withdraw lien.price that is 1e18.
3. Alice, who thought that the ParticleExchange.sellNftToMarket function would not allow an insolvent position to be opened, unexpectedly loses her position margin, which is 0.5e18, without having a chance to buy an NFT for repaying the lien.

Tools Used

VSCode

Recommended Mitigation Steps

The ParticleExchange.sellNftToMarket, ParticleExchange.swapWithEth, and ParticleExchange.refinanceLoan functions can be updated to check if the calculated credit of the respective lien is higher than a reasonable threshold value that is bigger than 0. If not, calling these functions should revert.

Assessed type

Other

[hansfriese]

It is the user's responsibility to keep the credit enough to sustain the loan as clearly explained in the protocol documentation.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-sponsor]

wukong-particle marked the issue as sponsor disputed

[wukong-particle]

Judge's screenshot is right. It's trader's responsibility to maintain a positive margin to avoid liquidation. If credit == 0 at position opening time, then, by design, lender is eligible to withdraw ETH immediately.

But we acknowledge the concern, will implement proper warning and messaging in our frontend.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid