No Duplicate Check could distribute more or all revenue to a holder

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

In the LiquidInfrastructureERC20 contract, an entitlement amount is added to the erc20EntitlementPerUnit array for a distributableERC20 (revenue token/reward).

If the contract owner had unknowingly used an array with duplicate entry during the deployment of the contract in https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L462 or during the call in setDistributableERC20s() , then this would create a situation where the distributableERC20 token has duplicate entitlements in the internal _beginDistribution() call, due to no existence check when adding entitlement to the erc20EntitlementPerUnit array.

Thus, above leading to an Infra ERC20 token holder being able to receive more rewards or all.

Proof of Concept

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L257-L281 contains the vulnerable logic

1. Contract Owner deploys LiquidInfrastructureERC20 contract but with array for distributableERC20 as [erc20TokenA, erc20TokenB, erc20TokenA]
2. Holders A and B are approved by Owner, and Infra ERC20 contract holds 1000000 erc20TokenA tokens for distribution.
3. Owner mints 100 tokens each to holders A and B by calling mint() function in respective order.
4. User calls distribute() with argument value 1.
5. All 1000000 tokens of the reward are distributed to holder A (being index 0 in holders array).

The test below is a PoC of the finding:

import chai from "chai";

import { mine } from "@nomicfoundation/hardhat-network-helpers";
import { ethers } from "hardhat";
import {
  deployContracts,
  deployERC20A,
  deployLiquidERC20,
  deployLiquidNFT,
} from "../test-utils";
import {
  TestERC20A,
  TestERC20B,
  TestERC20C,
  LiquidInfrastructureNFT,
  LiquidInfrastructureERC20,
} from "../typechain-types/contracts";
import { ERC20 } from "../typechain-types/@openzeppelin/contracts/token/ERC20";
import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers";
const {
  loadFixture,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");
const { expect } = chai;

const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
const ONE_ETH = 1000000000000000000;

// This test makes assertions about the LiquidInfrastructureERC20 contract by running it on hardhat
//
// Important test details:
// Contract interactions happen via hardhat-ethers: https://hardhat.org/hardhat-runner/plugins/nomiclabs-hardhat-ethers
// Chai is used to make assertions https://www.chaijs.com/api/bdd/
// Ethereum-waffle is used to extend chai and add ethereum matchers: https://ethereum-waffle.readthedocs.io/en/latest/matchers.html
async function liquidErc20Fixture() {
  const signers = await ethers.getSigners();
  const nftAccount1 = signers[0];
  const nftAccount2 = signers[1];
  const nftAccount3 = signers[2];
  const erc20Owner = signers[3];
  const holder1 = signers[4];
  const holder2 = signers[5];
  const holder3 = signers[6];
  const holder4 = signers[7];
  const badSigner = signers[8];
  const nonHolder = signers[9];

  // Deploy several ERC20 tokens to use as revenue currencies
  //////////////////
  const { testERC20A, testERC20B, testERC20C } = await deployContracts(
    erc20Owner
  );
  const erc20Addresses = [
    await testERC20A.getAddress(),
    await testERC20B.getAddress(),
    await testERC20A.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.totalSupply()).to.equal(0);
  expect(await infraERC20.name()).to.equal("Infra");
  expect(await infraERC20.symbol()).to.equal("INFRA");
  await expect(infraERC20.ManagedNFTs(0)).to.be.reverted;
  expect(await infraERC20.isApprovedHolder(holder1.address)).to.equal(false);
  await expect(infraERC20.mint(holder1.address, 1000)).to.be.reverted;
  expect(await infraERC20.balanceOf(holder1.address)).to.equal(0);

  return {
    infraERC20,
    testERC20A,
    testERC20B,
    testERC20C,
    signers,
    nftAccount1,
    nftAccount2,
    nftAccount3,
    erc20Owner,
    holder1,
    holder2,
    holder3,
    holder4,
    badSigner,
    nonHolder,
  };
}

export async function transferNftToErc20AndManage(
  infraERC20: LiquidInfrastructureERC20,
  nftToManage: LiquidInfrastructureNFT,
  nftOwner: HardhatEthersSigner
) {
  const infraAddress = await infraERC20.getAddress();
  const accountId = await nftToManage.AccountId();
  expect(await nftToManage.transferFrom(nftOwner, infraAddress, accountId)).to
    .be.ok;
  expect(await nftToManage.ownerOf(accountId)).to.equal(
    infraAddress,
    "unexpected nft owner"
  );

  expect(await infraERC20.addManagedNFT(await nftToManage.getAddress()))
    .to.emit(infraERC20, "AddManagedNFT")
    .withArgs(await nftToManage.getAddress());
}

async function basicDistributionTests(
  infraERC20: LiquidInfrastructureERC20,
  infraERC20Owner: HardhatEthersSigner,
  holders: HardhatEthersSigner[],
  nftOwners: HardhatEthersSigner[],
  nfts: LiquidInfrastructureNFT[],
  rewardErc20s: ERC20[]
) {
  const [holder1, holder2, holder3, holder4] = holders.slice(0, 4);
  const [nftOwner1, nftOwner2, nftOwner3] = nftOwners.slice(0, 3);
  let [nft1, nft2, nft3] = nfts.slice(0, 3);
  const [erc20a, erc20b, erc20c] = rewardErc20s.slice(0, 3);

  // Register one NFT as a source of reward erc20s
  await transferNftToErc20AndManage(infraERC20, nft1, nftOwner1);
  await mine(1);
  nft1 = nft1.connect(infraERC20Owner);

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

  // And then send the rewards to the ERC20
  await expect(infraERC20.withdrawFromAllManagedNFTs())
    .to.emit(infraERC20, "WithdrawalStarted")
    .and.emit(nft1, "SuccessfulWithdrawal")
    .and.emit(erc20a, "Transfer")
    .withArgs(
      await nft1.getAddress(),
      await infraERC20.getAddress(),
      rewardAmount1
    )
    .and.emit(infraERC20, "Withdrawal")
    .withArgs(await nft1.getAddress())
    .and.emit(infraERC20, "WithdrawalFinished");

  // Attempt to distribute with no holders
  await expect(infraERC20.distributeToAllHolders()).to.not.emit(
    infraERC20,
    "Distribution"
  );
  console.log("Reward Balance of holder1 before distribution: ", await erc20a.balanceOf(holder1.address));
  console.log("Reward Balance of holder2 before distribution: ", await erc20a.balanceOf(holder2.address));

  // Grant a holders 1 and 2 some of the Infra ERC20 tokens and then distribute all held rewards to them

  //Mint Infra ERC20 token to holder1
  await expect(infraERC20.mint(holder1.address, 100))
    .to.emit(infraERC20, "Transfer")
    .withArgs(ZERO_ADDRESS, holder1.address, 100);

    //mint for holder4
  await expect(infraERC20.mint(holder2.address, 100))
    .to.emit(infraERC20, "Transfer")
    .withArgs(ZERO_ADDRESS, holder2.address, 100);

  //fastforward to MinDistributionPeriod
  await mine(500);

  //distribute for 2 distributions (expectation was to distribute to holders 1 and 4)
  await expect(infraERC20.distribute(1))
    .to.emit(infraERC20, "DistributionStarted")
    .and.emit(infraERC20, "Distribution")
    .and.emit(erc20a, "Transfer")
    console.log("Reward Balance of holder1 after distribution: ", await erc20a.balanceOf(holder1.address));
    console.log("Reward Balance of holder2 after distribution: ", await erc20a.balanceOf(holder2.address));
//   await expect(infraERC20.distributeToAllHolders()).to.be.revertedWith("ERC20: transfer amount exceeds balance");
}

describe("No Duplicate Check could distribute more or all revenue to a holder", function () {
  it("manages distributions (basic)", async function () {
    const {
      infraERC20,
      erc20Owner,
      testERC20A,
      testERC20B,
      testERC20C,
      nftAccount1,
      nftAccount2,
      nftAccount3,
      holder1,
      holder2,
      holder3,
      holder4,
    } = await liquidErc20Fixture();

    const holders = [holder1, holder2, holder3, holder4];
    for (let holder of holders) {
      const address = holder.address;
      await expect(infraERC20.approveHolder(address)).to.not.be.reverted;
    }
    const nftOwners = [nftAccount1, nftAccount2, nftAccount3];
    let nfts: LiquidInfrastructureNFT[] = [
      await deployLiquidNFT(nftAccount1),
      await deployLiquidNFT(nftAccount2),
      await deployLiquidNFT(nftAccount3),
    ];
    const erc20s: ERC20[] = [testERC20A, testERC20B, testERC20C];
    for (const nft of nfts) {
      nft.setThresholds(
        erc20s,
        erc20s.map(() => 0)
      );
    }
    await basicDistributionTests(
      infraERC20,
      erc20Owner,
      holders,
      nftOwners,
      nfts,
      erc20s
    );
  });

});

Tools Used

Manual

Recommended Mitigation Steps

1. add a duplicate check before https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L276 in the internal _beginDistribution() call.
2. Or use a enumerableMap with a check.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #6

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Overinflated severity