If insider deposits and unlocks in quick sucession, attacker can steal their NFT and deposit funds

[zobront]

Summary

The dNFT contract allows the owner to mint a predefined quantity of "insider" NFTs without any deposit attached to them. These NFTs begin in a locked state, which stops them from being immediately liquidated due to their lack of deposits.

The protocol enforces that, in order for insider's to mint any DYAD, they must unlock their NFTs (so that they will be subject to liquidation, like all other users).

However, there is no safety check for the opposite case, where an insider unlocks their NFT before making a deposit. In this situation, any user could liquidate them and steal their NFT.

This is especially dangerous because if a user calls both of these functions in quick succession, they may both be in the mempool at the same time. If this is the case, a malicious attacker can create a flashbots bundle to sandwich their liquidation transaction between the unlock() and deposit() transactions, with the result that:

• The attacker will successfully liquidate and steal the insider's NFT
• The deposit transaction will deposit the insider's ETH to the stolen NFT, securing it for the attacker

Recommendation

I would recommend adding a check to the unlock() function to ensure this situation is avoided:

function unlock(uint id)
external
    isNftOwner(id)
{
    if (!id2Locked[id]) revert NotLocked();
+   if (id2Shared[id] == 0) revert MustDepositFirst();
    id2Locked[id] = false;
    emit Unlocked(id);
}

Note: This requires adding a MustDepositFirst() error to IDNft.sol.

[shafu0x]

there is no unlock mechanism anymore

[zobront]

Yep, this one is automatically fixed by the change in liquidation system, because 0 deposited and 0 withdrawn is no longer liquidatable.