Althea Liquid Infrastructure - Breeje's results

Lines of code

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

Vulnerability details

Proof of Concept

holders state variable tracks the addresses of all the holders of LiquidInfrastructureERC20 token.

distribute function iterates through this array to distribute the tokens.

To protect against Block Limit DoS issues, Owner holds the power to approve only limited number of holders and also partial distribution is also possible.

But there is an attack vector which allows unbounded duplicate addresses to be added in the holders array which can create several DoS related issues for the protocol.

Attack Vector:

_beforeTokenTransfer hook is used whenever the token is transferred, minted or burned.

Line a checks if the balanceOf to address is zero or not. If it not 0, exists = true but if it's 0, exists = false.

Line b adds to address to holder if exists = false meaning balanceOf(to) = 0.

    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();
        }

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

Another important point to note is that, 0 value transfers are allowed here.

While, the attack vector can cause issues both by malicious user behaviour and normal behaviour, but let's look at malicious behvaiour situation first by considering the following scenario:

1. Alice and Bob has 10 tokens each and wants to grief the system.
2. Alice transfers all 10 tokens to bob.
3. Bob then makes 0 value transfers to Alice.
4. At line a in _beforeTokenTransfer, exists will be false as Alice has 0 balance.
5. At line b, Alice's address will be added to holders array.
6. Bob can repeat this step any no. of time to make holders array so huge that functions which iterate through this (_afterTokenTransfer hook & distribute) will waste a lot of gas and will require to be called many times to complete one full cycle.

Potential Impacts:

1. Complete Transfers will be permanently blocked:

_afterTokenTransfer hook tracks where sender still holds any token or not. If not, it iterates through the entire holders array to pop out the sender address.

    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();
                }
            }
        }
    }

But in case, holders array is too large, it will always fail because of Block limit of Althea L1. Because of this, complete transfers of token from 1 address to other will be DoSed.

2. Not just malicious grief, but Burning tokens, which is normal behaviour, will add zero address to holder & increase it's size:

During burning of the token, _burn function calls _beforeTokenTransfer hook with from as account & to as zero address.

    function _burn(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: burn from the zero address");

@->     _beforeTokenTransfer(account, address(0), amount);
      
        // SNIP
    }

Important to note here that address(0) always has 0 balance.

Because of this, address: 0x0000000000000000000000000000000000000000 will always be added to holders to increase it's length unnecessarily everytime someone burns the token. This itself can become a huge issue & waste huge amount of gas during distribution.

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        // SNIP

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

Serious Extension of this Issue:

Most of the tokens reverts when transfering to address(0) (Any token using Openzeppelin ERC20 standard will revert). So any approved address can just burn a dust amount of token to block the distribution of following holders completely because of revert in the following line in distribute function. This is the maximum impact for this issue reported.

    for (i = nextDistributionRecipient; i < limit; i++) {
            address recipient = holders[i];
            if (isApprovedHolder(recipient)) {
                uint256[] memory receipts = new uint256[](distributableERC20s.length);
                for (uint j = 0; j < distributableERC20s.length; j++) {
                    IERC20 toDistribute = IERC20(distributableERC20s[j]);
                    uint256 entitlement = erc20EntitlementPerUnit[j] * this.balanceOf(recipient);
  @->               if (toDistribute.transfer(recipient, entitlement)) {
                        receipts[j] = entitlement;
                    }
                }

                emit Distribution(recipient, distributableERC20s, receipts);
            }
        }

3. Gas Wastage for protocol:

This is obvious issue whenever anyone calls distribute function. Given there is no bound on the amount of holders and potential for malicious/normal behaviour to increase it, this can also be serious issue for protocol.

Tools Used

VS Code

Recommended Mitigation Steps

Add a validation in _beforeTokenTransfer hook to revert whenever amount to be transferred is 0.

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
+       require(amount != 0, "Zero value transfer not allowed");

Assessed type

DoS

Judge has assessed an item in Issue #132 as 2 risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

Funds will be stuck in the contract as there are no recovery methods to take out shares of holders which owns the token but was disapproved at a later stage.

Proof of Concept

Main concept of LiquidInfrastructureERC20 is to distribute the revenue accured from NFTs to it’s holders.

Owner can use disapproveHolder to remove already approved holder from allowlist. It can be called anytime which is a normal behaviour.

    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
    }

During distribution, it is validated that only the approved holder can receive the tokens.

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

          emit Distribution(recipient, distributableERC20s, receipts);
      }

Now consider the following scenario:

1. Alice was approved initially and owns some LiquidInfrastructureERC20 tokens.
2. Owner wants to remove Alice because of some reason so Owner calls disapproveHolder which is a normal behaviour.
3. As Alice already owns the tokens, her share of distribution tokens will always be stuck in the contract which no one can ever withdraw.

Tools Used

VS Code

Recommended Mitigation Steps

I would recommend to add a function with onlyOwner access to withdraw the share of accounts who are unapproved but still owns the token.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Funds will be stuck in the contract as there are no recovery methods to take out shares of holders which owns the token but was disapproved at a later stage.

Proof of Concept

Main concept of LiquidInfrastructureERC20 is to distribute the revenue accured from NFTs to it's holders.

Owner can use disapproveHolder to remove already approved holder from allowlist. It can be called anytime which is a normal behaviour.

    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
    }

During distribution, it is validated that only the approved holder can receive the tokens.

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

          emit Distribution(recipient, distributableERC20s, receipts);
      }

Now consider the following scenario:

1. Alice was approved initially and owns some LiquidInfrastructureERC20 tokens.
2. Owner wants to remove Alice because of some reason so Owner calls disapproveHolder which is a normal behaviour.
3. As Alice already owns the tokens, her share of distribution tokens will always be stuck in the contract which no one can ever withdraw.

Tools Used

VS Code

Recommended Mitigation Steps

I would recommend to add a function with onlyOwner access to withdraw the share of accounts who are unapproved but still owns the token.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Permanent DoS of withdrawFromManagedNFTs functionality.

Proof of Concept

Withdrawing tokens from the NFT is the first step in the lifecycle of distribution of tokens to the holders.

When it comes to ManagedNFTs, owner holds the power to add or release it anytime. It is completely normal behaviour on owner's part.

But when owner is performing a normal operation of releasing couple of NFTs from the Contract, anyone can maliciously use an attack vector which will block all the future withdrawals permanently.

Attack Vector:

Consider the following scenario:

1. Assume, There exist 10 NFTs in ManagedNFTs.
2. The owner intends to release a couple of NFTs from the ManagedNFTs.
3. The owner initiates multiple calls specifying targeted NFT addresses and their beneficiaries.
4. A malicious user tracks these calls in mempool and executes a withdrawFromManagedNFTs call by frontrunning them, passing the value: initial length of ManagedNFTs - 1, i.e., 10 - 1 = 9.
5. Assuming the contract is currently unlocked for distribution, it proceeds to withdraw from the top 9 NFTs, updating the nextWithdrawal state variable to 9.
6. Subsequently, the owner's calls are executed, reducing the length of ManagedNFTs to 8.
7. Now, any attempt to call withdrawFromManagedNFTs will consistently fail to perform withdrawals, as the limit will always be min(numWithdrawal + 9, 8) = 8, causing the loop to iterate from i = 9 to i < 8, resulting in a loop that never executes and withdrawal can never happen (deadlock scenario).

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

        if (nextWithdrawal == 0) {
            emit WithdrawalStarted();
        }

@->     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();
        }
    }

Tools Used

VS Code

Recommended Mitigation Steps

To address this issue, I recommended to implement a require check ensuring that the withdrawal cycle is completed and not left partially done. This measure will protect the contract from entering a state of permanent DoS.

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
+       require(nextWithdrawal == 0, "Can't remove untill withdrawal cycle is completed");   
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        nft.transferFrom(address(this), to, nft.AccountId());

        // SNIP
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Proof of Concept

Partial withdrawal from NFTs are allowed to tackle potential Block limit DoS issue in the future.

But, consider the following scenario:

1. Assume, There exist 10 NFTs in ManagedNFTs.
2. The owner intends to release a NFT at index 3 from the ManagedNFTs.
3. Let's assume contract is currently in partial withdrawal state where nextWithdrawal = 6 meaning withdrawal from last 5 NFTs are still left.
4. In releaseManagedNFT function during owner's execution:

    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;
            }
        }

ManagedNFTs[3] will be replaced by ManagedNFTs[9] and position of ManagedNFTs[9] will be removed.

But in this case, Withdrawal from initial ManagedNFTs[9] got skipped as contract will assume it has already withdrawn from that NFT because of nextWithdrawal = 6.

Important point to note is that withdrawFromManagedNFTs is a public function, so any user holds the power to skip the last NFT withdrawal in the current round by frontrunning the owner's call.

Impact

Holders will be forced to wait for another MinDistributionPeriod to get the tokens from that NFT.

Tools Used

VS Code

Recommendation

To address this issue, I recommended to implement a require check in releaseManagedNFT function ensuring that the withdrawal cycle is completed and not left partially done. This measure will make sure that withdrawals from all NFTs takes place at the right time.

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
+       require(nextWithdrawal == 0, "Can't remove untill withdrawal cycle is completed");   
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        nft.transferFrom(address(this), to, nft.AccountId());

        // SNIP
    }

Assessed type

Invalid Validation