Althea Liquid Infrastructure - 0xlemon's results

Lines of code

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

Vulnerability details

Summary

The holders array can contain multiple zero addresses and grow so much that when looping through it in _afterTokenTransfer, it would cause an Out Of Gas. This means DoS to most of the LiquidInfrastructureERC20 logic like transferring tokens, burning or minting.

Vulnerability Details

The holders array may contain multiple zero addresses due to the conditional check within the _beforeTokenTransfer function. This check ensures that if the balance of the recipient address to is zero, the address is included in the holders array. However when burning a token the to address will be address(0) and this address doesn't hold any ERC20 tokens so it will be added in the holders array. This would happen before each burning and eventually the holders array will become big enough to cause an OOG error when transfering, burning or minting tokens because for all of these transactions _afterTokenTransfer will be called and it does loop through the holders array if the balance of the from address is 0.

It is important to mention that if a mint happens, then all of these zero addresses will be removed, however I think that this issue is valid because minting is supposed to happen first and it could never happen again as a lot of people would rather distributing a capped supply of tokens. Then multiple burnings can happen without a mint which would cause the error described above.

Proof of Concept

Add this test in the describe part of 2024-02-althea-liquid-infrastructure\liquid-infrastructure\test\liquidERC20.ts To run this test you should also add these two view functions in 2024-02-althea-liquid-infrastructure\liquid-infrastructure\contracts\LiquidInfrastructureERC20.sol since it is impossible to track the holders array without them:

function getHoldersLenght() public view returns (uint256) {
    return holders.length;
}

function getHolder(uint256 index) public view returns (address) {
    return holders[index];
}

Now the real test:

it("stuffs holders array with zero address after burn", async function() {
  const { infraERC20, holder1, holder2 } =
    await liquidErc20Fixture();

  await infraERC20.approveHolder(holder1.address);
  await infraERC20.mint(holder1.address, 1);

  await infraERC20.approveHolder(holder2.address);
  await infraERC20.mint(holder2.address, 1);

  const holder1InfraErc20 = await infraERC20.connect(holder1);
  await holder1InfraErc20.burn(1);

  expect(await infraERC20.getHoldersLenght()).to.equal(2);
  expect(await infraERC20.getHolder(0)).to.equal(ZeroAddress);

  const holder2InfraErc20 = await infraERC20.connect(holder2);
  await holder2InfraErc20.burn(1);

  expect(await infraERC20.getHoldersLenght()).to.equal(2);
  expect(await infraERC20.getHolder(0)).to.equal(ZeroAddress);
});

In this test we can clearly see how after the two holders burned their tokens, the holders array is still of length 2 and it contains two address(0).

This is just a demonstration with every holder holding only 1 ERC20, however in a real situation, the holders can hold multiple thousands, even millions of tokens and for every burning of just 1 of these tokens a new address(0) value will be added in the holders array.

Impact

Leading to complete DoS of LiquidInfrastructureERC20.

Tools Used

Manual Review, VS Code, Hardhat

Recommended Mitigation Steps

To mitigate this issue, in the _beforeTokenTransfer function when before adding the holder check if it is not zero address like that:

    bool exists = (this.balanceOf(to) != 0);
    if (!exists && to != address(0)) {
        holders.push(to);
    }

Assessed type

DoS