Althea Liquid Infrastructure - Tendency's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L116-L119 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L198-L237

Vulnerability details

Impact

According to the contract, all holders of LiquidInfrastructureERC20 infraERC20 tokens are expected to be approved, currently, when the contract owner disapproves a holder, the holder will no longer participate in any distribution of rewards. Holder disapproval is done via an owner call to LiquidInfrastructureERC20::disapproveHolder function.

Assuming we have three holders, Alice, Bobby and Charlie, with 100, 300, and 100 infraERC20 respectively. If Bobby is disapproved by the contract owner for some reason, and there is currently 1,000 USDC as a reward, yet to be distributed. During distribution erc20EntitlementPerUnit for the USDC is calculated as: entitlement = balance / supply where, contract balance = 1, 000 USDC infraERC20 supply = 500(the sum of all holders tokens) = 1,000 / 500 ==>2

During distribution, the reward is calculated as follows: erc20EntitlementPerUnit * user balance since the reward is only distributed to approved holders: Alice gets = 2 * 100 ==> 200 USDC Charlie gets = 2 * 100 ==> 200 USDC

400 USDC is distributed in total, and the 600 USDC(2 * 300) meant for Bobby is left stuck in the contract

It is worth noting that for every disapproved holder, the amount of stuck erc20 rewards in the contract increases.

According to the current implementation, I believe the expected behaviour is that all of the rewards be shared with only approved holders and since these holders don't get up to their expected reward amount, I believe this also qualifies as a loss of funds for the approved holders and thus am labelling the issue to be of high severity.

Proof of Concept

Please add the below to liquid-infrastructure/test/liquidERC20.ts

<details> <summary><b>LiquidERC20::LiquidInfrastructureERC20 tests</b></summary>

  it("Stuck Rewards", async function () {
    const { infraERC20, erc20Owner, testERC20A, nftAccount1 } =
      await liquidErc20Fixture();
    const bobby = "0x0000000000000000000000000000000000000001";
    const charlie = "0x0000000000000000000000000000000000000003";
    const alice = "0x0000000000000000000000000000000000000002";

    const holders = [bobby, charlie, alice];
    for (let holder of holders) {
      await expect(infraERC20.approveHolder(holder)).to.not.be.reverted;
    }
    // const nftOwners = [nftAccount1, nftAccount2, nftAccount3];
    let nft1: LiquidInfrastructureNFT = await deployLiquidNFT(nftAccount1);
    const erc20s: ERC20[] = [testERC20A];
    nft1.setThresholds(
      erc20s,
      erc20s.map(() => 0)
    );

    const [erc20a] = erc20s.slice(0, 1);

    //should all be zero balances
    expect(await erc20a.balanceOf(await infraERC20.getAddress())).to.equal(0);
    expect(await erc20a.balanceOf(bobby)).to.equal(0);
    expect(await erc20a.balanceOf(charlie)).to.equal(0);
    expect(await erc20a.balanceOf(alice)).to.equal(0);

    // Register one NFT as a source of reward erc20s
    await transferNftToErc20AndManage(infraERC20, nft1, nftAccount1);
    await mine(1);
    //Make next calls as owner
    nft1 = nft1.connect(erc20Owner);

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

    await infraERC20.withdrawFromAllManagedNFTs();

    //mint infra to the holders
    await infraERC20.mint(bobby, 300);
    await infraERC20.mint(charlie, 100);
    await infraERC20.mint(alice, 100);

    //disapprove bobby
    await infraERC20.disapproveHolder(bobby);

    expect(await infraERC20.isApprovedHolder(bobby)).to.be.false;

    //go past the minimum distribution period
    await mine(500);

    await infraERC20.distributeToAllHolders();

    expect(await erc20a.balanceOf(await infraERC20.getAddress())).to.equal(600); //600 USDC stays stuck in the contract
    expect(await erc20a.balanceOf(bobby)).to.equal(0); //bob gets nothing
    expect(await erc20a.balanceOf(charlie)).to.equal(200); //charlie and alice both get their share
    expect(await erc20a.balanceOf(alice)).to.equal(200);
  });

</details>

and then run:

npx hardhat test test/liquidERC20.ts

Tools Used

Manual Review & Hardhat

Recommended Mitigation Steps

Here is a comment from HolderAllowlist

This collection holds the whitelist for accounts approved to hold the LiquidInfrastructureERC20

From the comment, I believe all holders of these tokens must be approved, and thus, if a holder is disapproved, they should no longer be holding any of the tokens. I recommend burning all of a holder's tokens if they are ever disapproved, or updating the contract to keep a record of the balance of disapproved holders, when calculating entitlement this balance should then be deducted from the total supply, this way all the tokens for distribution will be sent to approved addresses

Assessed type

Error