Althea Liquid Infrastructure - jesjupyter's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L106-L109 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L216 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L270-L276

Vulnerability details

Impact

If a holder is disapproved by the owner using disapproveHolder, his will not be eligible in distribute due to the isApprovedHolder check. However, in _beginDistribution, his share is still taken into account when calculating erc20EntitlementPerUnit. This will lead to incorrect distribution, since other eligible users may receive fewer rewards, and some rewards are still left in the contract for the next round of distribution.

Proof of Concept

The owner can use disapproveHolder to prevent some users from receiving any more of the underlying ERC20. This could work for someone who is already possessing some underlying ERC20 tokens.

    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
    }

In the distribute function, holder can't receive rewards since he is not eligible.

            if (isApprovedHolder(recipient)) {
                uint256[] memory receipts = new uint256[](
                    distributableERC20s.length
                );
                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;
                    }
                }

                emit Distribution(recipient, distributableERC20s, receipts);
            }

However, when the distribution starts, the erc20EntitlementPerUnit is calculated using entitlement = balance / supply in _beginDistribution.

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

Consider the following situation:

1. User A, B, C both have 1 LiquidInfrastructureERC20.
2. There are 30 USDC in the contract to distribute.
3. User A is disapproved and will not participate in distribute.
4. In calculation, erc20EntitlementPerUnit for USDC will be 10 USDC per LiquidInfrastructureERC20 token since A's share is still counted in the calculation.
5. Thus, in distribute, B and C will only receive 10 USDC instead of 15 USDC. And 10 USDC is left in the contract to be distributed in the next round.

Tools Used

VSCode

Recommended Mitigation Steps

Add a variable frozen account to record the share of LiquidInfrastructureERC20 being frozen due to disapproval. And use the substrated value supply-frozen to calculate entitlement.

    
    uint frozen = 0;

    function approveHolder(address holder) public onlyOwner {
        require(!isApprovedHolder(holder), "holder already approved");
        HolderAllowlist[holder] = true;
        frozen -= this.balanceOf(holder); // <= reduce when the holder is approved
    }

    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false; 
        frozen += this.balanceOf(holder); // <= increase when the holder is disapproved
    }

    function _beginDistribution() internal {
        ...

        // Calculate the entitlement per token held
        uint256 supply = this.totalSupply();
        require(supply > frozen, "no user to distribute"); // <= add here

        for (uint i = 0; i < distributableERC20s.length; i++) {
            uint256 balance = IERC20(distributableERC20s[i]).balanceOf(
                address(this)
            );
            uint256 entitlement = balance / (supply-frozen); // <= modify code here
            erc20EntitlementPerUnit.push(entitlement);
        }

        nextDistributionRecipient = 0;
        emit DistributionStarted();
    }

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L106-L109 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

Impact

approveHolder and disapproveHolder should not be called during distribution. If a holder is disapproved, some rewards will be left in the contract and will not be used in this round of distribution.

Proof of Concept

There is no check of LockedForDistribution in the function approveHolder and disapproveHolder. This means that a user can be disapproved during the distribution process.

    function disapproveHolder(address holder) public onlyOwner {
        require(isApprovedHolder(holder), "holder not approved");
        HolderAllowlist[holder] = false;
    }

Consider the following scenario:

1. User A, B, and C is ready for distribution.
2. User A is distributed.
3. Later user B is disapproved, so he will not get his share of rewards.
4. After C is distributed, some rewards are still left in the contract, leading to incorrect distribution.

Tools Used

Manual

Recommended Mitigation Steps

Add require(!LockedForDistribution," cannot disapprove/approve when already locked"); in the function approveHolder and disapproveHolder.

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L441-L445 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L272-L276 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/bd6ee47162368e1999a0a5b8b17b701347cf9a7d/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L221-L225

Vulnerability details

Impact

In the setDistributableERC20s function, the distributableERC20s is reset by the owner without any check on LockedForDistribution . If the function is called after the distribution has begun, this would cause inconsistency between distributableERC20s and erc20EntitlementPerUnit, leading to Denial of Service or incorrect rewards distribution.

Proof of Concept

In the setDistributableERC20s function, the distributableERC20s is reset by the owner. Thus tokens and length may all change during the function call.

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

However, since there is no check on LockedForDistribution, the function setDistributableERC20s could be called during the distribution process.

In _beginDistribution however, erc20EntitlementPerUnit will be created for each token in distributableERC20s.

        for (uint i = 0; i < distributableERC20s.length; i++) {
            uint256 balance = IERC20(distributableERC20s[i]).balanceOf(
                address(this)
            );
            uint256 entitlement = balance / supply;
            erc20EntitlementPerUnit.push(entitlement);
        }

If setDistributableERC20s after _beginDistribution, this would cause inconsistency in tokens and length. And would cause unintended behavior in distribute since erc20EntitlementPerUnit[j] is inconsistent with distributableERC20s[j].

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

In this scenario, the transaction would revert if erc20EntitlementPerUnit.length < distributableERC20s.length or there is not enough balance to transfer for the new token. Otherwise, the distribution would be completely incorrect(For example, 3 USDT is distributed, but should be 3 WETH instead).

Tools Used

Manual, VSCode

Recommended Mitigation Steps

Add require(!LockedForDistribution," cannot set distributableERC20s when already locked"); in the function setDistributableERC20s.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The redundant requirement will never revert and should be removed.

Proof of Concept

The requirement require(true, "unable to find released NFT in ManagedNFTs"); is useless in the code since it will never revert.

Tools Used

Manual

Recommended Mitigation Steps

Remove the requirement.

Assessed type

Other