Althea Liquid Infrastructure - juancito's results

Lines of code

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

Vulnerability details

Impact

• When adding tokens: Prevents any future calls to distribute(), which will revert with a panic error. Bricks the contract as it will try to access an out of bound index in the erc20EntitlementPerUnit[] array.
• When removing tokens: It bricks the distribution of tokens, leading to distributing a different amount than expected, or reverting.

Vulnerability Details

The root of the vulnerability is setDistributableERC20s() not checking that there is an ongoing distribution:

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

LiquidInfrastructureERC20.sol#L441-L445

The scenario goes as follows:

1. Anyone can call distribute() at any moment with a number of distributions lower than the total holders.
2. The contract will be locked for distribution during the begin phase of the distribution.
3. A temporary erc20EntitlementPerUnit[] array is set in storage with the same length as distributableERC20s[] (this will be the cause of the revert).
4. As there will pending recipients, the distribution will not be ended, and the lock will not be released.
5. The owner then calls setDistributableERC20s(), which will update the distributableERC20s[] array.
6. When a new distribution is attempted, it won't execute the begin distribution, as it assumes there is an ongoing one.
7. It will then revert, as the erc20EntitlementPerUnit[] array will try to access an index out of bounds, since it will iterate over a larger array.

Note: Someone could even frontrun the tx to purposefully perform the attack, although this is not necessary for the brick to happen.

Another scenario can be removing a token during distribution, or changing its order. This will mess up the distribution, as erc20EntitlementPerUnit is first initiated expecting the same positions of the erc20 token array, but then when distribution continues it will have different values than expected, as the indexes will differ.

Proof of Concept

Add the following test to liquid-infrastructure/test/liquidERC20.ts.

It will revert with panic code 0x32 (Array accessed at an out-of-bounds or negative index)

describe("POC", function() {
  it.only("bricks the contract when updating tokens during a distribution", async function() {
    const { infraERC20, holder1, holder2, holder3, badSigner, testERC20A, testERC20B, testERC20C } = await liquidErc20Fixture();

    // Setup the contract with two ERC20 tokens
    const tokenAddresses = [await testERC20A.getAddress(), await testERC20B.getAddress()];
    await infraERC20.setDistributableERC20s(tokenAddresses);

    // Approve holders in order to be able to mint them tokens
    await infraERC20.approveHolder(holder1.address);
    await infraERC20.approveHolder(holder2.address);
    await infraERC20.approveHolder(holder3.address);

    // Mint tokens to holders so there are users to distribute to
    await infraERC20.mint(holder1.address, 1000)
    await infraERC20.mint(holder2.address, 1000)
    await infraERC20.mint(holder3.address, 1000)

    // Advance blocks to a distributable time
    await mine(500);

    // Anyone can start a distribution at any time
    const infraERC20NotOwner = infraERC20.connect(badSigner);
    await infraERC20NotOwner.distribute(1);

    // The project adds a new ERC20
    const newAddresses = [await testERC20A.getAddress(), await testERC20B.getAddress(), await testERC20C.getAddress()];
    await infraERC20.setDistributableERC20s(newAddresses);

    // It will brick the contract
    // This will revert with "panic code 0x32 (Array accessed at an out-of-bounds or negative index)"
    await infraERC20.distribute(1);
  })
});

Recommended Mitigation Steps

This lines prevents any possible brick:

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Removed tokens that were not distributed before updating the ERC20 tokens list will be locked. Holders will not receive their expected tokens.

Vulnerability Details

Tokens are only distributed based on the current distributableERC20s[] array:

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

LiquidInfrastructureERC20.sol#L220-L227

The problem is that if a token is removed via setDistributableERC20s(), they can't later be distributed and will remain locked in the contract.

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

LiquidInfrastructureERC20.sol#L441-L445

Recommended Mitigation Steps

Make sure tokens are distributed before removing any token.

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

DOS of withdrawFromManagedNFTs() and withdrawFromAllManagedNFTs() functions

Vulnerability Details

The problem is that withdrawFromManagedNFTs() checks that nextWithdrawal is exactly equal to the length of managed NFTs.

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

LiquidInfrastructureERC20.sol#L382-L385

nextWithdrawal is an always increasing value (the only place where it is reset is in the previous code snippet).

    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;

This means that if for some reason nextWithdrawal > ManagedNFTs.length, the value will never be reset, leading to a DOS of the withdraw function, as it won't be able to iterate over the array again.

Releasing NFTs removes an item from the ManagedNFTs array.

When two or more NFTs are released, it can lead to the previous scenario.

Someone could even frontrun the tx to purposefully perform the attack by withdrawing ManagedNFTs.length - 1 items, although this is not strictly necessary for the brick to happen.

Recommended Mitigation Steps

One option is to reset nextWithdrawal when nextWithdrawal >= ManagedNFTs.length. This way it will reset the value on the next withdraw call.

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

Another option is to prevent releasing NFTs if there is an ongoing withdrawal. This check could be added to addManagedNFT() for consistency as well.

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
+       require(nextWithdrawal == 0);

Assessed type

DoS