Althea Liquid Infrastructure - kutugu's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/2b26c04f0448505635210416bef9d3ce96391b16/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L221-L225

Vulnerability details

Impact

Token distribution is non-atomic, erc20EntitlementPerUnit is calculated first and the cached value is always used during token distribution. If setDistributableERC20s is called during the distribution, old erc20EntitlementPerUnit data will cause the wrong number of tokens to be distributed.

Proof of Concept

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

import {Test, console2} from "forge-std/Test.sol";
import {LiquidInfrastructureERC20, ERC20} from "../contracts/LiquidInfrastructureERC20.sol";

contract ERC20Test is Test {
    LiquidInfrastructureERC20 public erc20;
    uint256 constant MIN_PERIOD = 10;
    address constant HOLDER2 = address(0xdead);

    function setUp() public {
        address[] memory managedNFTs = new address[](0);
        address[] memory approvedHolders = new address[](2);
        approvedHolders[0] = address(this);
        approvedHolders[1] = HOLDER2;
        address[] memory distributableErc20s = new address[](0);
        erc20 = new LiquidInfrastructureERC20(
            "ERC20", "E",
            managedNFTs,
            approvedHolders,
            MIN_PERIOD,
            distributableErc20s
        );
        erc20.mint(address(this), 1e18);
        erc20.mint(HOLDER2, 1e18);
    }

    function testWrongUnitAfterSetDistributableERC20s() public {
        vm.roll(block.number + MIN_PERIOD);
        ERC20 token0 = new ERC20("Token0", "T0");
        ERC20 token1 = new ERC20("Token1", "T1");
        address[] memory distributableErc20s1 = new address[](2);
        distributableErc20s1[0] = address(token0);
        distributableErc20s1[1] = address(token1);
        uint256 firstDistribute = 10e18;
        deal(address(token0), address(erc20), firstDistribute);
        deal(address(token1), address(erc20), firstDistribute * 2);
        erc20.setDistributableERC20s(distributableErc20s1);
        erc20.distribute(1);
        assertEq(token0.balanceOf(address(this)), firstDistribute / 2);
        assertEq(token1.balanceOf(address(this)), firstDistribute);

        vm.roll(block.number + MIN_PERIOD);
        ERC20 token2 = new ERC20("Token2", "T2");
        address[] memory distributableErc20s2 = new address[](1);
        distributableErc20s2[0] = address(token2);
        deal(address(token2), address(erc20), firstDistribute * 100);
        erc20.setDistributableERC20s(distributableErc20s2);
        erc20.distribute(1);
        // @audit should be firstDistribute / 50, not firstDistribute / 2
        assertEq(token2.balanceOf(HOLDER2), firstDistribute / 2);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

erc20EntitlementPerUnit should be reset when call setDistributableERC20s

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/2b26c04f0448505635210416bef9d3ce96391b16/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L366-L371 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/2b26c04f0448505635210416bef9d3ce96391b16/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L425-L426

Vulnerability details

Impact

releaseManagedNFT will decrease ManagedNFTs.length, and withdrawFromManagedNFTs caches nextWithdrawal, which depends on ManagedNFTs.length. This causes nextWithdrawal to be stale and DOS withdrawFromManagedNFTs.

Proof of Concept

// withdrawFromManagedNFTs
uint256 limit = Math.min(
    numWithdrawals + nextWithdrawal,
    ManagedNFTs.length
);
uint256 i;
for (i = nextWithdrawal; i < limit; i++) {
    // ...
}

// releaseManagedNFT
ManagedNFTs[i] = ManagedNFTs[ManagedNFTs.length - 1];

If the nextWithdrawal of withdrawFromManagedNFT caches ManagedNFTs.length before releaseManagedNFT, after releaseManagedNFT is called, nextWithdrawal may be lower than the latest ManagedNFTs.length, causing the withdrawFromManagedNFTs function to be unavailable.
Another issue is that because releaseManagedNFT will exchange array index, the execution order of withdrawFromManagedNFTs will be disordered.

The following is a POC for DOS:

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

import {Test, console2} from "forge-std/Test.sol";
import {LiquidInfrastructureERC20, ERC20} from "../contracts/LiquidInfrastructureERC20.sol";
import {LiquidInfrastructureNFT} from "../contracts/LiquidInfrastructureNFT.sol";

contract ERC20Test is Test {
    LiquidInfrastructureERC20 public erc20;
    uint256 constant MIN_PERIOD = 10;
    address constant HOLDER2 = address(0xdead);

    function setUp() public {
        address[] memory managedNFTs = new address[](0);
        address[] memory approvedHolders = new address[](2);
        approvedHolders[0] = address(this);
        approvedHolders[1] = HOLDER2;
        address[] memory distributableErc20s = new address[](0);
        erc20 = new LiquidInfrastructureERC20(
            "ERC20", "E",
            managedNFTs,
            approvedHolders,
            MIN_PERIOD,
            distributableErc20s
        );
        erc20.mint(address(this), 1e18);
        erc20.mint(HOLDER2, 1e18);
    }

    function testReleaseManagedNFTDOSWithdrawFromManagedNFTs() public {
        vm.startPrank(address(erc20));
        LiquidInfrastructureNFT nft1 = new LiquidInfrastructureNFT("NFT1");
        LiquidInfrastructureNFT nft2 = new LiquidInfrastructureNFT("NFT2");
        LiquidInfrastructureNFT nft3 = new LiquidInfrastructureNFT("NFT3");
        LiquidInfrastructureNFT nft4 = new LiquidInfrastructureNFT("NFT4");
        LiquidInfrastructureNFT nft5 = new LiquidInfrastructureNFT("NFT5");
        vm.stopPrank();

        erc20.addManagedNFT(address(nft1));
        erc20.addManagedNFT(address(nft2));
        erc20.addManagedNFT(address(nft3));
        erc20.addManagedNFT(address(nft4));
        erc20.addManagedNFT(address(nft5));
        erc20.withdrawFromManagedNFTs(4);
        assertEq(erc20.nextWithdrawal(), 4);

        erc20.releaseManagedNFT(address(nft3), address(this));
        erc20.releaseManagedNFT(address(nft4), address(this));
        erc20.withdrawFromAllManagedNFTs();
        // @audit no more changes
        assertEq(erc20.nextWithdrawal(), 4);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

1. releaseManagedNFT should reset nextWithdrawal if nextWithdrawal >= ManagedNFTs.length
2. releaseManagedNFT should not swap array index

Assessed type

Context