Althea Liquid Infrastructure - d3e4'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

One can add duplicates of an address to holders, which address will then receive duplicate entitlements (stolen from other holders) during a distribution, and brick the contract.

Proof of Concept

Transfers of LiquidInfrastructureERC20 executes the following code in _beforeTokenTransfer(from, to, amount):

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

That is, if to had a zero balance then to is added to holders. The expectation is that when a holder's balance goes from zero to non-zero the holder is added to holder. But if amount == 0 his balance remains zero and if this zero amount is transferred again to to then to will be added again to holders. This can be repeated to add an arbitrary number of copies of to in holders. (Note that _afterTokenTransfer() only pops the from, which also implies that this zero transfer must be from an address with non-zero balance (which does not have to be controlled by the attacker since a zero amount is transferred).) (After this duplication, a non-zero amount can be sent or minted to the address.)

distribute(), over one or several calls,

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

iterates over each address (recipient) in holders,

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

sends an entitlement in proportion to its balance of ,

            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;

and ends the distribution only when all addresses in holders are covered.

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

For the attacker's duplicated recipient this.balanceOf(recipient) will of course always retrieve the same value, but he will, each time, still receive a full entitlement based on this balance. He will thus receive a multiple of what he should be entitled to. This will be at the expense of other holders, and since distribute() attempts to distribute the entire balances of distributableERC20s at some nextDistributionRecipient < holders.length the funds will run out. This means that the distribution cannot end, and since the distribution process locks all transfers, mints, burns, and withdrawals, this leaves the contract permanently locked (unless funds are donated to the contract, in which case the attacker can just continue stealing from it).

Recommended Mitigation Steps

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

distributableERC20s can be modified during distribution, potentially causing distribute() to revert or sending incorrect entitlements to holders.

Proof of Concept

A distribution may be split up in several calls to distribute(). To avoid inconsistencies between these calls the variables outside the logical scope of this distribution, holders and distributableERC20s, must remain constant until the distribution is completed. holders is kept constant by blocking _beforeTokenTransfer() during a distribution. However, distributableERC20s can always be modified:

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

When a distribution is started a list erc20EntitlementPerUnit is filled according to the indexing of the ERC20s in distributableERC20s, in _beginDistribution().

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

distribute() contains the following:

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

We notice two problems. If distributableERC20s has been made longer, such that distributableERC20s.length > erc20EntitlementPerUnit.length then erc20EntitlementPerUnit[j] will revert due to out-of-bounds. If the order of the ERC20s has changed in distributableERC20s then distributableERC20s[j] will not correspond to erc20EntitlementPerUnit[j] and the wrong entitlement will be sent to the holder(s).

Recommended Mitigation Steps

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

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

If managed NFTs are released during a partial withdrawal from the managed NFTs it may happen that nextWithdrawal > ManagedNFTs.length which blocks any further withdrawal attempts.

Proof of Concept

LiquidInfrastructureERC20.sol#L359-L386:

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

Suppose ManagedNFTs.length == 10 and that a partial withdrawal has been made, e.g. withdrawFromManagedNFTs(9) has been called. This sets limit = 9 and thus leaves nextWithdrawal = 9. Then suppose two managed NFTs are released, by calling releaseManagedNFT() twice. This pops two elements from ManagedNFTs so that now ManagedNFTs.length == 8. Now, the next time withdrawFromManagedNFTs() is called limit = 8, so the for-loop is skipped. But now nextWithdrawal == 9 and ManagedNFTs.length == 8 so nextWithdrawal != ManagedNFTs.length and nextWithdrawal fails to be reset to 0. Calling withdrawFromManagedNFTs() does nothing and is stuck in this state, preventing withdrawals.

Recommended Mitigation Steps

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

Assessed type

DoS