Althea Liquid Infrastructure - Krace's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L127-L146 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#L198-L237

Vulnerability details

Impact

The _beforeTokenTransfer function does not verify whether the transfer amount is zero, thereby enabling any user to add a holder to holders by transferring zero tokens to a valid holder. The only requirement is that the valid holder must have zero balances before the transfer. By adding holders, anyone can increase the length of holders, potentially resulting in a denial-of-service (DoS) situation due to Out Of Gas, as both _afterTokenTransfer and distribute may iterate through the entire holders.

Proof of Concept

In the _beforeTokenTransfer function, to is added to holders if it is a valid holder and has a balance equal to zero before the transfer. However, it does not check whether the transfer amount is zero. Consequently, even if the transferred amount is 0, as long as the balance of to is also 0, to will be added to holders. Although to is added to holders, its balance remains zero, implying that to could be added to holders again.

    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();
        }
        //@audit `to` is added to holders as long as its balance is zero. 
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }

This vulnerability allows anyone to exploit the situation and add elements to holders, potentially causing a Denial-of-Service (DoS) situation due to Out of Gas, as both _afterTokenTransfer and distribute iterate through the entire holders.

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

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

To validate the aforementioned issue, I conducted a test where holder1 represents an unauthorized holder, and holder2 is an authorized holder. In this scenario, holder1 is able to continually add new holders to the holders list by repeatedly transferring 0 tokens to holder2.

Apply the patch and run it with npx hardhat test.

diff --git a/liquid-infrastructure/test/liquidERC20.ts b/liquid-infrastructure/test/liquidERC20.ts
index 0121500..73577d1 100644
--- a/liquid-infrastructure/test/liquidERC20.ts
+++ b/liquid-infrastructure/test/liquidERC20.ts
@@ -176,6 +176,31 @@ async function failToManageNFTNotOwner(
   );
 }

+async function emptyHolder(
+  infraERC20: LiquidInfrastructureERC20,
+  holder1: HardhatEthersSigner,
+  holder2: HardhatEthersSigner,
+  badSigner: HardhatEthersSigner
+) {
+
+  // Holder1 is not a valid holder
+  const infraERC20Holder1 = infraERC20.connect(holder1);
+
+  // Approve holder2
+  await expect(infraERC20.approveHolder(holder2.address)).to.not.be.reverted;
+
+  // Transfer 0 erc20 from holder1 to holder2 ==> two holders
+  await expect(infraERC20Holder1.transfer(holder2.address, 0)).to.not.be
+    .reverted;
+  var x = await infraERC20.getHolders();
+  console.log("holders length ",x);
+ // Transfer 0 erc20 from holder1 to holder2 again ==> three holders
+  await expect(infraERC20Holder1.transfer(holder2.address, 0)).to.not.be
+    .reverted;
+  x = await infraERC20.getHolders();
+  console.log("holders length ",x);
+}
+
 // Checks that only owner-approved holders are allowed to hold the ERC20,
 // and that even the owner cannot give them tokens without approving them
 async function basicErc20HolderTests(
@@ -529,6 +554,13 @@ describe("LiquidInfrastructureERC20 tests", function () {
     await basicErc20HolderTests(infraERC20, holder1, holder2, badSigner);
   });

+  it.only("empyt holders", async function () {
+    const { infraERC20, holder1, holder2, badSigner } =
+      await liquidErc20Fixture();
+
+    await emptyHolder(infraERC20, holder1, holder2, badSigner);
+  });
+
   it("manages distributions (basic)", async function () {
     const {
       infraERC20,

This diff add a view function to LiquidInfrastructureERC20.sol to get the length of holders.

diff --git a/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol b/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
index 4722279..834cf4e 100644
--- a/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
+++ b/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
@@ -145,6 +145,9 @@ contract LiquidInfrastructureERC20 is
         }
     }

+function getHolders() public view returns (uint256){
+    return holders.length;
+}
     /**
      * Implements an additional lock on minting and burning, ensuring that supply changes happen after any potential distributions
      */

Tools Used

Hardhat

Recommended Mitigation Steps

Check that the amount is not zero in _beforeTransferToken before adding the to to holders.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L127-L146 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#L198-L237

Vulnerability details

Impact

The _beforeTokenTransfer function does not verify whether the address of to is equal to zero when adding new holders, thereby enabling any user to add a holder of address zero to holders by burning tokens. By adding holders, anyone can increase the length of holders, potentially resulting in a denial-of-service (DoS) situation due to Out Of Gas, as both _afterTokenTransfer and distribute may iterate through the entire holders.

Proof of Concept

In the _beforeTokenTransfer function, the logic adds to to holders if it is a valid holder with a balance equal to zero before the transfer. During token burning, to is set to zero, which bypasses the isApprovedHolder(to) check. The exists check only examines the balance of to but not its address. Since the balance of ZERO is always zero, to will consistently be added to holders when tokens are burned.

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        require(!LockedForDistribution, "distribution in progress");
        //@audit when burn, `to` is zero, which will not check the approval.
        if (!(to == address(0))) {
            require(
                isApprovedHolder(to),
                "receiver not approved to hold the token"
            );
        }
        if (from == address(0) || to == address(0)) {
            _beforeMintOrBurn();
        }
        //@audit `to` is added to holders even if `to` is zero. 
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }

This vulnerability allows anyone to exploit the situation and add elements to holders, potentially causing a Denial-of-Service (DoS) situation due to Out of Gas, as both _afterTokenTransfer and distribute iterate through the entire holders. Attackers don't even need to be approved holders, they can simply call burn(0) to add new holders.

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

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

To validate the aforementioned issue, I conducted a test where holder1 represents an unauthorized holder. In this scenario, holder1 is able to continually add new holders (Address ZERO) to the holders list by repeatedly burn zero tokens.

Apply the patch and run it with npx hardhat test.

diff --git a/liquid-infrastructure/test/liquidERC20.ts b/liquid-infrastructure/test/liquidERC20.ts
index 0121500..5d948b5 100644
--- a/liquid-infrastructure/test/liquidERC20.ts
+++ b/liquid-infrastructure/test/liquidERC20.ts
@@ -176,6 +176,27 @@ async function failToManageNFTNotOwner(
   );
 }

+
+async function BurnToAddHolders(
+  infraERC20: LiquidInfrastructureERC20,
+  holder1: HardhatEthersSigner,
+  holder2: HardhatEthersSigner,
+  badSigner: HardhatEthersSigner
+) {
+
+  // Holder1 is not a valid holder
+  const infraERC20Holder1 = infraERC20.connect(holder1);
+
+  var x = await infraERC20.getHolders();
+  console.log("holders length ",x);
+
+  await infraERC20Holder1.burn(0);
+  x = await infraERC20.getHolders();
+  console.log("holders length ",x);
+  await infraERC20Holder1.burn(0);
+  x = await infraERC20.getHolders();
+  console.log("holders length ",x);
+}
 // Checks that only owner-approved holders are allowed to hold the ERC20,
 // and that even the owner cannot give them tokens without approving them
 async function basicErc20HolderTests(
@@ -529,6 +550,13 @@ describe("LiquidInfrastructureERC20 tests", function () {
     await basicErc20HolderTests(infraERC20, holder1, holder2, badSigner);
   });

+  it.only("burn to add holders", async function () {
+    const { infraERC20, holder1, holder2, badSigner } =
+      await liquidErc20Fixture();
+
+    await BurnToAddHolders(infraERC20, holder1, holder2, badSigner);
+  });
+
   it("manages distributions (basic)", async function () {
     const {
       infraERC20,

This diff add a view function to LiquidInfrastructureERC20.sol to get the length of holders.

diff --git a/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol b/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
index 4722279..834cf4e 100644
--- a/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
+++ b/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
@@ -145,6 +145,9 @@ contract LiquidInfrastructureERC20 is
         }
     }

+function getHolders() public view returns (uint256){
+    return holders.length;
+}
     /**
      * Implements an additional lock on minting and burning, ensuring that supply changes happen after any potential distributions
      */

Result:

  LiquidInfrastructureERC20 tests
holders length  0n
holders length  1n
holders length  2n
    ✔ burn to add holders (650ms)


  1 passing (653ms)

Tools Used

Hardhat

Recommended Mitigation Steps

Check that the to is not a zero address in function _beforeTransferToken before adding the to to holders.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

If the owner changes distributableERC20s during the distribute process, certain holders may receive an incorrect amount of tokens.

Proof of Concept

distribute calls _beginDistribution to calculate the erc20EntitlementPerUnit if the process of distribution has just begun. And it will distribute the erc20 to the holders according to the distributableERC20s and the calculated erc20EntitlementPerUnit.

    function distribute(uint256 numDistributions) public nonReentrant {
        require(numDistributions > 0, "must process at least 1 distribution");
        //@audit If the distribute is just begin, calculate the Unit.
        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
                );
                //@audit distribute the erc20 according to the distributableERC20s and Unit
                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();
        }
    }

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

        // clear the previous entitlements, if any
        if (erc20EntitlementPerUnit.length > 0) {
            delete erc20EntitlementPerUnit;
        }

        // Calculate the entitlement per token held
        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);
        }

        nextDistributionRecipient = 0;
        emit DistributionStarted();
    }

However, the owner could change the distributableERC20s even if the distribute is not complete.

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

Considering the following scenarios:

• distributableERC20s: [USDC, USDT, DAI], holders: [1,2,3] with same balances of LiquidInfrastructureERC20.
• distribute(1) calls _beginDistribution to initialize the erc20EntitlementPerUnit to [10, 20, 30] and transfers corresponding balances to holders1 ==> [10 USDC, 20 USDT, 30 DAI]
• setDistributableERC20s sets the distributableERC20s to [DAI, USDT, USDC]. The erc20EntitlementPerUnit is still [10, 20, 30].
• distribute(2) will transfer incorrect value of tokens to holder2 and holder3 because the order of distributableERC20s has been changed ==> [10 DAI, 20 USDT, 30 USDC].

Tools Used

Manual Review

Recommended Mitigation Steps

Disallow the execution of setDistributableERC20s during the ongoing process of distribute until it is completed.

diff --git a/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol b/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
index 4722279..6c86579 100644
--- a/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
+++ b/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol
@@ -441,6 +441,7 @@ contract LiquidInfrastructureERC20 is
     function setDistributableERC20s(
         address[] memory _distributableERC20s
     ) public onlyOwner {
+        require(!LockedForDistribution, "distribution in progress");
         distributableERC20s = _distributableERC20s;
     }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

If the owner uses releaseManagedNFT during the withdrawFromManagedNFTs process, certain ManagedNFTs may be unable to withdraw their balances to this contract. This action could potentially result in a denial-of-service (DoS) scenario for the withdrawFromManagedNFTs function, especially if the length of the new ManagedNFTs is less than the current nextWithdrawal.

Proof of Concept

withdrawFromManagedNFTs iterates through the ManagedNFTs and withdraws balances accordingly. It utilizes nextWithdrawal to keep track of the index of the next NFT's ID for withdrawal.

    function withdrawFromManagedNFTs(uint256 numWithdrawals) public {
        require(!LockedForDistribution, "cannot withdraw during distribution");

        if (nextWithdrawal == 0) {
            emit WithdrawalStarted();
        }

        uint256 limit = Math.min(
            numWithdrawals + nextWithdrawal,
            ManagedNFTs.length
        );
        uint256 i;
        for (i = nextWithdrawal; i < limit; i++) {
            LiquidInfrastructureNFT withdrawFrom = LiquidInfrastructureNFT(
                ManagedNFTs[i]
            );

            (address[] memory withdrawERC20s, ) = withdrawFrom.getThresholds();
            withdrawFrom.withdrawBalancesTo(withdrawERC20s, address(this));
            emit Withdrawal(address(withdrawFrom));
        }
        nextWithdrawal = i;

        if (nextWithdrawal == ManagedNFTs.length) {
            nextWithdrawal = 0;
            emit WithdrawalFinished();
        }
    }

However, releaseManagedNFT has the capability to remove the ManagedNFTs at any time, even if the process of withdrawFromManagedNFTs is not finished (i.e., when nextWithdrawal is not zero). This could potentially lead to unexpected situations.

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
        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);
    }

Considering two scenarios:

Scenario 1:

• ManagedNFTs: [1,2,3,4]
• withdrawFromManagedNFTs(2) withdraws balances from 1 and 2, setting nextWithdrawal to 2.
• releaseManagedNFT(2, anyaddress) removes 2 from ManagedNFTs, resulting in ManagedNFTs=[1,3,4].
• withdrawFromManagedNFTs(2) withdraws balances from 4 because the element at index 2 in ManagedNFTs is 4.
• Element 3 is ignored.

Scenario 2:

• ManagedNFTs: [1,2,3,4]
• withdrawFromManagedNFTs(2) withdraws balances from 1, 2, and 3, setting nextWithdrawal to 3.
• releaseManagedNFT(1, anyaddress) removes 1 from ManagedNFTs, resulting in ManagedNFTs=[2,3,4].
• releaseManagedNFT(2, anyaddress) removes 2 from ManagedNFTs, resulting in ManagedNFTs=[3,4].
• withdrawFromManagedNFTs(2) cannot withdraw new balances because nextWithdrawal = 3 is greater than the length of ManagedNFTs.
• This can only be recoverd when the owner add new NFTs.

Tools Used

Manual Review

Recommended Mitigation Steps

Disallow the execution of releaseManagedNFT during the ongoing process of withdrawFromManagedNFTs until it is completed.

Assessed type

Context