Withdrawals can be bricked if releasing more than one NFT during ongoing withdrawal

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L382-L385

Vulnerability details

Impact

DOS of withdrawFromManagedNFTs() and withdrawFromAllManagedNFTs() functions

Vulnerability Details

The problem is that withdrawFromManagedNFTs() checks that nextWithdrawal is exactly equal to the length of managed NFTs.

    if (nextWithdrawal == ManagedNFTs.length) {
        nextWithdrawal = 0;
        emit WithdrawalFinished();
    }

LiquidInfrastructureERC20.sol#L382-L385

nextWithdrawal is an always increasing value (the only place where it is reset is in the previous code snippet).

    uint256 limit = Math.min(
👉      numWithdrawals + nextWithdrawal,
        ManagedNFTs.length
    );
    uint256 i;
👉  for (i = nextWithdrawal; i < limit; i++) {
        LiquidInfrastructureNFT withdrawFrom = LiquidInfrastructureNFT(ManagedNFTs[i]);

        (address[] memory withdrawERC20s, ) = withdrawFrom.getThresholds();
        withdrawFrom.withdrawBalancesTo(withdrawERC20s, address(this));
        emit Withdrawal(address(withdrawFrom));
    }
👉  nextWithdrawal = i;

This means that if for some reason nextWithdrawal > ManagedNFTs.length, the value will never be reset, leading to a DOS of the withdraw function, as it won't be able to iterate over the array again.

Releasing NFTs removes an item from the ManagedNFTs array.

When two or more NFTs are released, it can lead to the previous scenario.

Someone could even frontrun the tx to purposefully perform the attack by withdrawing ManagedNFTs.length - 1 items, although this is not strictly necessary for the brick to happen.

Recommended Mitigation Steps

One option is to reset nextWithdrawal when nextWithdrawal >= ManagedNFTs.length. This way it will reset the value on the next withdraw call.

-   if (nextWithdrawal == ManagedNFTs.length) {
+   if (nextWithdrawal >= ManagedNFTs.length) {
        nextWithdrawal = 0;
        emit WithdrawalFinished();
    }

Another option is to prevent releasing NFTs if there is an ongoing withdrawal. This check could be added to addManagedNFT() for consistency as well.

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
+       require(nextWithdrawal == 0);

Assessed type

DoS

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #130

[c4-judge]

0xA5DF marked the issue as satisfactory