DoS Attack caused by large holders' array in LiquidInfrustractureERC20.sol

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The _afterTokenTransfer() function in the LiquidInfrustractureERC20 contract is vulnerable to Denial of Service (DoS) attacks. When the holders array becomes excessively large, it can block all transfer, burn, and mint operations, leading to a complete DoS attack on the contract.

Proof of Concept

The _afterTokenTransfer() function is called after every transfer, burn, or mint operation in the LiquidInfrustractureERC20 contract. This function iterates through the holders array, which can reach the gas limit and cause a DoS attack when the array size is large.

  /**
     * Removes `from` from the list of holders when they no longer hold any balance
     * @param from token sender
     * @param to  token receiver
     * @param amount  amount sent
     */
    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        bool stillHolding = (this.balanceOf(from) != 0);
        if (!stillHolding) {
            for (uint i = 0; i < holders.length; i++) {
                if (holders[i] == from) {
                    // Remove the element at i by copying the last one into its place and removing the last element
                    holders[i] = holders[holders.length - 1];
                    holders.pop();
                }
            }
        }
    }

It is important to note that the scenario of having an excessive number of holders is plausible. Specifically, the developers have addressed this issue in the distribute() function by iterating over only a portion of the holders array (from index nextDistributionRecipient to min( nextDistributionRecipient + numDistributions, holders.length ) in each transaction, thereby preventing DoS attacks due to gas limit constraints.

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

        uint256 limit = Math.min(
            nextDistributionRecipient + numDistributions,
            holders.length
        );

        uint i;
        for (i = nextDistributionRecipient; i < limit; i++) { //@audit: only handle a slice of holders in a single transaction
            address recipient = holders[i];
            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);
            }
        }
        nextDistributionRecipient = i;

        if (nextDistributionRecipient == holders.length) {
            _endDistribution();
        }
    }

However, the _afterTokenTransfer() function currently lacks similar protection schemes as the distrubte() function, leading to DoS attacks caused by a large holders array.

Tools Used

Manual Analysis

Recommended Mitigation Steps

It is recommended to use a mapping instead of an array to store holder information. For example:

mapping(address => bool) IsHolder

And change the _beforeTokenTransfer() and_afterTokenTransfer() function as follows

// Use a mapping to store holder information
mapping(address => bool) isHolder;

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) {
        isHolder[to] = true; 
    }
}

function _afterTokenTransfer(
    address from,
    address to,
    uint256 amount
) internal virtual override {
    bool stillHolding = (this.balanceOf(from) != 0);
    if (!stillHolding) {
        isHolder[from] = false;
    }
}

Assessed type

DoS

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #729

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moving discussion to #423