Althea Liquid Infrastructure - PumpkingWok's results

Lines of code

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

Vulnerability details

Impact

LiquidInfrastructureERC20 can't withdraw any ERC20 from managed NFTs.

Proof of Concept

In some scenarios, if more than one NFT managed contracts are released, the withdraw can be stuck until at least one NFT managed is added.

// 4 NFT managed contracts added
    function test_withdraw() public {
        // Withdraw from all except the last one
        liERC20.withdrawFromManagedNFTs(3);
    
        uint256 nextWithdrawal = liERC20.nextWithdrawal(); // 3
        
        // release 2 managed NFTs
        liERC20.releaseManagedNFT(address(liNFT1), address(this));
        liERC20.releaseManagedNFT(address(liNFT2), address(this));

        // withdraw nr withdrawals -> 1
        liERC20.withdrawFromManagedNFTs(1);
        nextWithdrawal = liERC20.nextWithdrawal(); // remain 3

        // withdraw nr withdrawals -> 2
        liERC20.withdrawFromManagedNFTs(2);
        nextWithdrawal = liERC20.nextWithdrawal(); // remain 3

        // withdraw nr withdrawals -> 0
        liERC20.withdrawFromManagedNFTs(0);
        nextWithdrawal = liERC20.nextWithdrawal(); // remain 3

        // Withdraw() is in stuck until at least a new managed NFT will be added

        liNFT1.transferFrom(address(this), address(liERC20), 1);
        liERC20.addManagedNFT(address(liNFT1));

        liERC20.withdrawFromManagedNFTs(1);
        nextWithdrawal = liERC20.nextWithdrawal(); // reset to 0      
    }

Here in the test, 4 NFT managed contracts have been added. The first withdraw function withdraws from all except the last one, then 2 NFT managed are removed. At this point the nextWithdrawal = 3 and ManagedNFTs.length = 2, so the limit calculated in withdrawFromManagedNFTs(numWithdrawal) will be always 2, the min between the managed nft length and numWithdrawals + nextWithdrawal, whatever will be the numWithdrawal pass as input param. the next for loop will be always skipped because nextWithdrawal > limit, and the nextWithdrawal won't change. It can be "fixed" adding at least one nft managed so ManagedNFTs.length > 2 and the for loop can be executed at least one time to reset nextWithdrawal .

Tools Used

Foundry

Recommended Mitigation Steps

An easy solution can be to include a check within releaseManagedNFT(). It may revert if nextWithdrawal != 0.

Assessed type

Token-Transfer