Althea Liquid Infrastructure - zhaojohnson's results

Judge has assessed an item in Issue #308 as 3 risk. The relevant finding follows:

• [L03] Addresses can be pushed into holders even if balanceOf(addr) is zero. For example, Alice sends 0 LiquidInfrastructureERC20 Token to Bob. Although Bob owns 0 Token, Bob addr can still be added in holders.

    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();
        }
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }

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#L214-L231

Vulnerability details

Impact

revenue tokens cannot be distributed completely because of disapproved holders

Proof of Concept

In current implementation, disapproved holders cannot receive any more underlying ERC20 tokens. And they may hold some LiquidInfrastructureERC20 tokens, which may influence erc20EntitlementPerUnit calculation.

For example:

• There are three holders: Alice(10 Tokens), Bob(10 Tokens), Cathy(10 Tokens)
• In normal case, all three holders are approved holders and everyone will get some profits.
• If Bob and Cathy are disapproved, Alice, as the only approved holder, she should earn all revenue tokens. However, in the calculation of entitlement, we use the totalSupply(), so Alice can only get 1/3 rewards.

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

Tools Used

Manual

Recommended Mitigation Steps

We need to add one state to monitor activeSupply for all approved holders.

Assessed type

Math

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

Vulnerability details

Impact

owner can call setDistributableERC20s in the distribution, which may cause dos

Proof of Concept

In the start of distribution, system will calculate erc20EntitlementPerUnit based on current distributableERC20s. If owner changes distributableERC20s in the process of distribution, token distribution may be wrong.

For example,

• Current distribution ERC20s are [USDC]
• Alice starts one distribute(x) and now length of erc20EntitlementPerUnit is 1
• Owner calls setDistributableERC20s() to update ERC20s from [USDC] to [USDC, USDT]
• Anyone want to go on finishing distribute(), distribute() will revert because of missing erc20EntitlementPerUnit[1]

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

Tools Used

Manual

Recommended Mitigation Steps

Disable setDistributableERC20s() in the process of distribute

Assessed type

DoS

• [L01] invalid require() in function releaseManagedNFT()

  • Need to add some conditions in require() to check whether this is valid release or not.

    function releaseManagedNFT(
        address nftContract,
        address to
    ) public onlyOwner nonReentrant {
        LiquidInfrastructureNFT nft = LiquidInfrastructureNFT(nftContract);
        nft.transferFrom(address(this), to, nft.AccountId());
  
        // Remove the released NFT from the collection
        for (uint i = 0; i < ManagedNFTs.length; i++) {
            address managed = ManagedNFTs[i];
            if (managed == nftContract) {
                // Delete by copying in the last element and then pop the end
                ManagedNFTs[i] = ManagedNFTs[ManagedNFTs.length - 1];
                ManagedNFTs.pop();
                break;
            }
        }
        // By this point the NFT should have been found and removed from ManagedNFTs
        require(true, "unable to find released NFT in ManagedNFTs");
  
        emit ReleaseManagedNFT(nftContract, to);
    }

• [L02] Add some check in constructor about _managedNFTs. In Function addManagedNFT(), if we want to add one managedNFT, we have one check to make sure the ownership of NFT. In order to the consistency, we'd better to add similar check in constructor()

    constructor(
        string memory _name,
        string memory _symbol,
        address[] memory _managedNFTs,
        address[] memory _approvedHolders,
        uint256 _minDistributionPeriod,
        address[] memory _distributableErc20s
    ) ERC20(_name, _symbol) Ownable() {
        //@johnson,[L] in addManagedNFT, when add one NFT, we need to check ownership, suggest do the same check here
        ManagedNFTs = _managedNFTs;

• [L03] Addresses can be pushed into holders even if balanceOf(addr) is zero. For example, Alice sends 0 LiquidInfrastructureERC20 Token to Bob. Although Bob owns 0 Token, Bob addr can still be added in holders.

    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();
        }
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }