Althea Liquid Infrastructure - Limbooo's results

Lines of code

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

Vulnerability details

Description

The LiquidInfrastructureERC20 contract implements a mechanism to disapprove token holders from receiving further distributions. When the owner calls the disapproveHolder function, disapproved holders lose their entitlement to future distributions. However, the existing tokens share held by these disapproved holders remain in the contract, leading to an accumulation of tokens over time.

Impact

The accumulation of tokens from disapproved holders can result in an imbalance in the distribution of rewards among remaining token holders. Disapproved holders' tokens are effectively locked in the contract, which may affect the fairness and effectiveness of the distribution mechanism. Moreover, the accumulation of unused tokens may also impact the overall token economics. The accumulated tokens can not be retrieved even by the owner.

Proof of Concept

1. (Foundry Setup) Add require("@nomicfoundation/hardhat-foundry") in hardhat.config.js and run this to be able to run the PoC:

npm install --save-dev @nomicfoundation/hardhat-foundry
npx hardhat init-foundry

2. Create a test/LiquidInfrastructureERC20Test.t.sol file containing the code below:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import {Test, console} from "forge-std/Test.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {LiquidInfrastructureERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";

contract LiquidInfrastructureERC20Test is Test {
    LiquidInfrastructureERC20 public liqInfraERC20;
    address erc20Owner = makeAddr("ERC20_Owner");
    address alice = makeAddr("Alice");
    address bob = makeAddr("Bob");
    
    
    address[] erc20Addresses;
    address[] managedNFTs;
    address[] approvedHolders;

    function setUp() public {      
        // Deploy ERC20 contract and LiquidInfrastructureERC20 contract
        erc20Addresses = new address[](1);
        erc20Addresses[0] = address(new MockERC20());

        vm.startPrank(erc20Owner);
        liqInfraERC20 = new LiquidInfrastructureERC20(
            "Infra",
            "INFRA",
            managedNFTs,
            approvedHolders,
            500,
            erc20Addresses
        );

        // Approve holders and mint tokens, for Alice and Bob
        liqInfraERC20.approveHolder(alice);
        liqInfraERC20.mint(alice, 50);

        liqInfraERC20.approveHolder(bob);
        liqInfraERC20.mint(bob, 50);
        vm.stopPrank();

    }

    function test_PoC_DisapproveHolderSharesAccumulateTokens() public {
        MockERC20 erc20 = MockERC20(erc20Addresses[0]);

        // Mint 100 tokens to the ERC20 contract held by LiquidInfrastructureERC20
        erc20.mint(address(liqInfraERC20), 100);

        // Roll the block to trigger the distribution period
        vm.roll(501);

        // Ensure the ERC20 contract balance in LiquidInfrastructureERC20 is 100
        assertEq(erc20.balanceOf(address(liqInfraERC20)), 100);
        // Ensure Alice and Bob have 0 balance before the distribution
        assertEq(erc20.balanceOf(alice), 0);
        assertEq(erc20.balanceOf(bob), 0);

        // Distribute rewards to all holders (Alice and Bob)
        liqInfraERC20.distributeToAllHolders();
        // Ensure the ERC20 contract balance becomes 0 after distribution
        assertEq(erc20.balanceOf(address(liqInfraERC20)), 0);
        // Ensure Alice and Bob receive 50 tokens each
        assertEq(erc20.balanceOf(alice), 50);
        assertEq(erc20.balanceOf(bob), 50);

        // Disapprove Bob as a holder
        vm.startPrank(erc20Owner);
        liqInfraERC20.disapproveHolder(bob);
        vm.stopPrank();

        // Mint another 100 tokens to the ERC20 contract held by LiquidInfrastructureERC20
        erc20.mint(address(liqInfraERC20), 100);

        // Roll the block to trigger the next distribution period
        vm.roll(1001);
        // Distribute rewards to all holders (Alice)
        liqInfraERC20.distributeToAllHolders();
        // The ERC20 contract balance dose not becomes 0 after distribution!
        assertNotEq(erc20.balanceOf(address(liqInfraERC20)), 0);
        // Alice has 100 tokens (before has 50 + receives only 50), 
        // Bob has only 50 (did not receives anything since he is disapproved)
        assertEq(erc20.balanceOf(alice), 100);
        assertEq(erc20.balanceOf(bob), 50);

        // Mint 1000 more tokens to the ERC20 contract held by LiquidInfrastructureERC20
        erc20.mint(address(liqInfraERC20), 1000);
        vm.roll(1501);
        // Distribute rewards to all holders (Alice)
        liqInfraERC20.distributeToAllHolders();
        // The accumulation of tokens held by LiquidInfrastructureERC20 will get bigger now
        assertEq(erc20.balanceOf(address(liqInfraERC20)), 550);
        // Alice receives only 500 only while he is the only approved holder!
        assertEq(erc20.balanceOf(alice), 600);
        assertEq(erc20.balanceOf(bob), 50);
    }
}


contract MockERC20 is ERC20 {
    constructor() ERC20("ERC20", "ERC20") {}
    function mint(address to, uint256 value) public {
        _mint(
            to,
            value
        );
    }
}

3. Run the test with forge test --mt test_PoC_DisapproveHolderSharesAccumulateTokens and see this test passing.

Tools Used

Foundry

Recommended Mitigation Steps

To mitigate the accumulation of tokens from disapproved holders consider one of the following:

• Update the distribution mechanism to exclude disapproved holders from the entitlement calculation entirely. Modify the _beginDistribution() function to calculate entitlements only for approved holders.

• Implement token burning functionality within the disapproveHolder function. Upon disapproval, burn the tokens held by the disapproved holder to remove them from circulation. This ensures that disapproved holders not only lose their entitlement to future distributions but also forfeit their existing tokens, preventing an accumulation of unused tokens in the contract.

Modify the disapproveHolder function to include token burning functionality:

function disapproveHolder(address holder) public onlyOwner {
    require(isApprovedHolder(holder), "holder not approved");

    // Burn the tokens held by the disapproved holder
    uint256 balanceToBurn = balanceOf(holder);
    _burn(holder, balanceToBurn);

    // Mark the holder as disapproved
    HolderAllowlist[holder] = false;
}

With this modification, disapproved holders' tokens will be burned, ensuring a fairer distribution of rewards among remaining token holders and preventing an accumulation of unused tokens in the contract.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In LiquidInfrastructureERC20 contract, The _beginDistribution function locks the contract for distribution and calculates the entitlement to protocol-held ERC20s per unit (distributableERC20s) of the LiquidInfrastructureERC20 held. Changing the distributableERC20s array while the contract is locked for distribution can lead to incorrect distribution calculations, potential loss of funds, and unexpected behavior during reward distribution to token holders.

Proof of Concept

Consider the following scenarios:

1. Adding Tokens Mid-Distribution:
   • Description: Owner adds new ERC20 tokens to the distributableERC20s array while a distribution is ongoing.
   • Impact: Tokens that were not originally included in the distribution might be distributed, potentially leading to an incorrect allocation of rewards. Also, this could generate out-of-pound issue in erc20EntitlementPerUnit array.
2. Removing Tokens Mid-Distribution:
   • Description: Owner removes ERC20 tokens from the distributableERC20s array while a distribution is ongoing.
   • Impact: Tokens that were supposed to be distributed might be excluded, leading to an incorrect allocation of rewards.

Recommended Mitigation

Consider preventing modification during distribution, Implement a check within the setDistributableERC20s function to ensure that it cannot be called while the contract is locked for distribution.

function setDistributableERC20s(address[] memory _distributableERC20s) public onlyOwner {
    require(!LockedForDistribution, "Cannot update while locked for distribution");
    distributableERC20s = _distributableERC20s;
}

Assessed type

Other