`newLien.lender` can steal NFT that should belong to `oldLien.lender` after refinancing

[code423n4]

Lines of code

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#L172-L189

Vulnerability details

Impact

After calling the following ParticleExchange.refinanceLoan function, collection are the same and tokenId become newLien.tokenId for both the old and new liens.

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) {
        ...

        if (oldLien.collection != newLien.collection) {
            // cannot swap to a new loan with different collection
            revert Errors.UnmatchedLoans();
        }

        /// @dev cannot have negative credit (will revert if new credit < 0)
        // since: old credit = sold amount + position margin - oldLien.price
        // and:   new credit = (sold amount + position margin - interest) - newLien.price
        // hence: new credit = (old credit + oldLien.price - interest) - newLien.price
        uint256 payableInterest = _calculateCurrentPayableInterest(oldLien);
        uint256 newCredit = (oldLien.credit + oldLien.price - payableInterest) - newLien.price;

        // update old lien
        liens[oldLienId] = keccak256(
            abi.encode(
                Lien({
                    lender: oldLien.lender,
                    borrower: address(0),
                    collection: oldLien.collection,
                    tokenId: newLien.tokenId,
                    price: oldLien.price,
                    rate: oldLien.rate,
                    loanStartTime: 0,
                    credit: 0,
                    auctionStartTime: 0
                })
            )
        );

        // 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
                })
            )
        );

        // accure interest to the lender
        interestAccrued[oldLien.lender] += payableInterest;

        ...
    }

In this situation, because both oldLien.lender and newLien.lender are the lenders of the liens for the same NFT, which corresponds to the same collection-tokenId combination, newLien.lender can call the following ParticleExchange.withdrawNftWithInterest function to withdraw such NFT. Then, calling the ParticleExchange.withdrawNftWithInterest function by oldLien.lender reverts since the ParticleExchange contract no longer owns such NFT. As a result, oldLien.lender loses the NFT that should belong to it after the refinancing.

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

    function withdrawNftWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
        if (msg.sender != lien.lender) {
            revert Errors.Unauthorized();
        }

        // delete lien
        delete liens[lienId];

        // transfer NFT back to lender
        /// @dev can withdraw means NFT is currently in contract without active loan,
        /// @dev the interest (if any) is already accured to lender at NFT acquiring time
        IERC721(lien.collection).safeTransferFrom(address(this), msg.sender, lien.tokenId);

        emit WithdrawNFT(lienId);

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

Proof of Concept

The following steps can occur for the described scenario.

1. When calling the ParticleExchange.refinanceLoan function, Alice is oldLien.lender, Bob is newLien.lender, oldLien.collection and newLien.collection are Milady, and newLien.tokenId is 8621.
2. After calling the ParticleExchange.refinanceLoan function, both the old and new liens are for Milady 8621.
3. Bob calls the ParticleExchange.withdrawNftWithInterest function to withdraw Milady 8621.
4. Calling the ParticleExchange.withdrawNftWithInterest function by Alice reverts. Hence, Alice loses Milady 8621 that should belong to her.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2023-05-particle/blob/bbd1c01407a017046c86fdb483bbabfb1fb085d8/contracts/protocol/ParticleExchange.sol#L593-L607 can be updated to the following code, where tokenId is set to oldLien.tokenId instead of newLien.tokenId.

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

Assessed type

Token-Transfer

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-sponsor]

wukong-particle marked the issue as sponsor acknowledged

[wukong-particle]

Good finding. The suggested fix is counter-intuitive for developer though, the new lien shouldn't take over old lien's tokenId (it's already out of the protocol).

The solution should be that the newLien shouldn't be able to withdrawNft, because it's in active loan. The fix from https://github.com/code-423n4/2023-05-particle-findings/issues/13 (check if it's active loan) should suffice to fix this issue here too.

[c4-sponsor]

wukong-particle marked the issue as sponsor confirmed

[c4-sponsor]

wukong-particle marked the issue as sponsor acknowledged

[c4-judge]

hansfriese marked the issue as duplicate of #13

[c4-judge]

hansfriese marked the issue as not a duplicate

[hansfriese]

As the sponsor pointed out, the essential vulnerability lies in the lack of validation for the function withdrawNftWithInterest and the suggested mitigation from the warden is not accepted.

Nullifying this one with issue #39 from the same warden in mind.

[c4-judge]

hansfriese marked the issue as nullified

[wukong-particle]

The solution should be that the newLien shouldn't be able to withdrawNft, because it's in active loan. The fix from https://github.com/code-423n4/2023-05-particle-findings/issues/13 (check if it's active loan) should suffice to fix this issue here too.

Fixed with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/2