Althea Liquid Infrastructure - JohnSmith's results

Lines of code

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

Vulnerability details

Impact

holders can contain same address multiple times, when a user have two approved addresses or just two approved users, they can push their addresses to the array by transfering 0 amount to approved address which has 0 balance of LiquidInfrastructureERC20 token. It is possible because we do not check if address to add is not already in the array:

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

Whe do the distribution contract does not have enough of distributable ERC20s amount to transfer because we count each duplicate in the array. Hence transaction is reverted. Distribution impossible to execute. Money stuck in the contract.

Proof of Concept

To add an address to holders array all we need is to be approved and have zero balance. Second approved address can just transfer 0 amount to first address, increasing quantity of holders. It is enough even to add just one duplicate. I made a test case which you can add to your liquid-infrastructure/test/liquidERC20.ts tests

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/test/liquidERC20.ts#L494
...
describe("LiquidInfrastructureERC20 tests", function () {
+  it.only("attack distributions", async function () {
+    const {
+      infraERC20,
+      testERC20A,
+      holder4,
+      holder2,
+    } = await liquidErc20Fixture();
+
+    const holders = [holder4, holder2];
+    for (let holder of holders) {
+      const address = holder.address;
+      await expect(infraERC20.approveHolder(address)).to.not.be.reverted;
+    }
+
+    let erc20rewardsAmount = 1000;
+    const infraAddress = await infraERC20.getAddress();
+    await testERC20A.transfer(infraAddress, erc20rewardsAmount);
+  
+    await infraERC20.mint(holder4.address, 10);
+    await infraERC20.mint(holder2.address, 10);
+   
+    await mine(500);
+    //optional test to make sure we setup everything correctly
+    await expect(infraERC20.distributeToAllHolders()).to.not.be.reverted;
+  
+    await testERC20A.transfer(infraAddress, erc20rewardsAmount);
+  
+    const infraERC20Holder4 = await infraERC20.connect(holder4);
+    await infraERC20Holder4.transfer(holder2.address, 10);
+    const infraERC20Holder2 = await infraERC20.connect(holder2);
+    await infraERC20Holder2.transfer(holder4.address, 0);// after this holders array consists of three elements instead of two 
+    await infraERC20Holder2.transfer(holder4.address, 10);
+  
+    await mine(500);
+    await expect(infraERC20.distributeToAllHolders()).to.be.revertedWith("ERC20: transfer amount exceeds balance");
+  });

In my test above assume we 1000 reward tokens from managed nft, or someone transfered, does not matter. Assume we have two holders, each has 10 LiquidInfrastructureERC20 tokens, which is 50% share for each one. when holder4 transfers all his tokens or burns them he can be added to holders array again when someone transfers or mints(looking at you mr trusted owner) tokens to him, while he has zero balance. After adding him two times and giving back his 10 tokens, we have three elements in holders. Next contract tries do distribute tokens but fails because when we count erc20EntitlementPerUnit we get 1000 / 20, which is 50. Then we try to transfer money to holders, which we have three now(two duplicates + one), we get the situation where we need 500 tokens for each, which is 1500, when we have only 1000 on contract balance. That is why distribution transaction fails.

About frontrunning Attacker can frontrun minting transaction to insert his approved addresses before someone new become added to holders. So if we have 10 tokens and new user got 100 tokens, attacker frontruns and adds 10 his addresses(same address 10 times), before new holder, So on distribution his addresses are processed before new holders. For distribution to not fail instead of function distributeToAllHolders() public attacker will call function distribute(uint256 numDistributions) public nonReentrant with right numDistributions so it processes his addresses, and new holder's address on next function call, that way attacker gets money and new holder gets transaction reverted. Since root cause is the same I did not write a test case for this one.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check to deny zero amount transactions like this:

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        ....
+        require(amount != 0, "zero amount not allowed");

Or/and add if address already present in holders

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        ...
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
+            bool present;
+            for(uint i; i < holders.length; ++i) {
+                if (holders[i] == from) {
+                    present = true;
+                }
+            }
+            if (!present) {
                holders.push(to);
+            }
        }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When LiquidInfrastructureERC20 contract owner disapproves a holder with tokens, disapproved holder's share of erc20s future distributions become locked in the contract.

Proof of Concept

Let's assume Alice and Bob are only two approved holders, each has 10 LiquidInfrastructureERC20 tokens. Contract owner disapproves Bob. Now we have approved holder Alice with 10 tokens and disapproved holder Bob with 10 tokens. Contract has 100 usd worth of erc20s to distribute. Time to distribute. We want all gained erc20s be transfered to approved holders. Which is only Alice. Assume we have only one type of erc20 to distribute, so

LiquidInfrastructureERC20.sol
257:        function _beginDistribution() internal {
                ....
275:            uint256 entitlement = balance / supply;
276:            erc20EntitlementPerUnit.push(entitlement);

entitlement per unit of distributable erc20 will be 100 divided by 20 total supply, which makes us 5 usd per LiquidInfrastructureERC20 token. Then in

LiquidInfrastructureERC20.sol
198:        function distribute(uint256 numDistributions) public nonReentrant {
                ....
222:            uint256 entitlement = erc20EntitlementPerUnit[j] *
223:                this.balanceOf(recipient);
224:            if (toDistribute.transfer(recipient, entitlement)) {

we calculate entitlement and transfer it, where erc20EntitlementPerUnit[j] is 5 and this.balanceOf(recipient) is 10 5*10=50, Alice as only approved holder gets only 50 usd, other 50 just stays on LiquidInfrastructureERC20 contract balance, locked.

Tools Used

Manual review

Recommended Mitigation Steps

If Bob loses access to his wallet address or become malicious, he will not burn his tokens, so I suggest burn tokens on disaproval

LiquidInfrastructureERC20.sol
    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
+       _burn(holder, this.balanceOf(holder));
    }

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

When withdrawFromManagedNFTs(uint256 numWithdrawals) executed in multiple transactions, changing ManagedNFTs.length will cause inability to withdraw ERC20s from manged NFTs.

Proof of Concept

In a situation when we execute withdrawFromManagedNFTs multiple times to withdraw erc20 from all managed NFTs, to finish withdrawal we need to satisfy the condition

LiquidInfrastructureERC20.sol
382:    if (nextWithdrawal == ManagedNFTs.length)

The nextWithdrawal becomes equal to ManagedNFTs.length when we finish for loop for last time. However if ManagedNFTs.length is reduced between withdrawFromManagedNFTs(uint256 numWithdrawals) calls, we can end up in a situation where nextWithdrawal can be bigger then ManagedNFTs.length. This is possible when releaseManagedNFT is called, it pops an element, reduces length of ManagedNFTs

        LiquidInfrastructureERC20.sol
413:    function releaseManagedNFT(
            ...
426:        ManagedNFTs.pop();

Tools Used

Manual review

Recommended Mitigation Steps

Before changing ManagedNFTs.length ensure we are not in a process of ERC20s withdrawal

LiquidInfrastructureERC20.sol
    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
+       require(nextWithdrawal == 0, "withdrawal in progress")
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        nft.transferFrom(address(this), to, nft.AccountId());

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

        emit ReleaseManagedNFT(nftContract, to);
    }

Assessed type

DoS