Althea Liquid Infrastructure - gesha17's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L127 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L164 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6edb6dd1ca43d05a762d84c688116b3327f5e490/contracts/token/ERC20/ERC20.sol#L251 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6edb6dd1ca43d05a762d84c688116b3327f5e490/contracts/token/ERC20/ERC20.sol#L274

Vulnerability details

Impact

If the holders array in LiquidInfrastructure.sol becomes too big, the _afterTokenTransfer() function will hit the block gas limit and revert on a token transfer where the sender sends all of their tokens or burns all of their tokens. This will also cause the mint() function to revert due to it hitting the block gas limit, because it also triggers the _afterTokenTransfer() function with sender being address(0) and an amount of 0.

Over time, as users interact with the protocol, the size of the holders array will grow naturally, increasing gas cost per every mint and ultimately causing transactions to hit the block gas limit. Furthermore an approved user can send 0 tokens to all approved users that have a balance of 0 and add them to the holders array, essentially gas griefing or even dossing. There is no way to artificially reduce the size of the array, so the mint function can be evetually permanently dossed.

Proof of Concept

The purpose of the holders array is to keep track of which users have any amount of the current token. The array is unbounded and it grows on every transfer to a new account. When a user wants to transfer the underlying ERC20, the _beforeTokenTransfer() function is called, then the actual transfer function and then the _afterTokenTransfer() function. Both _beforeTokenTransfer() and _afterTokenTransfer() access and manipulate the holders array.

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

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

As we can see if the account has a balance of 0 before the transfer, it is added to the holders array. Furthermore, there are no checks on the amount being 0, so a malicios user does not need to own or expend tokens to add more addresses to the array. For the purpose of gas griefing they just need to be approved users with a balance of 0.

Lets take a look the _afterTokenTransfer() function:

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

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

As a measure to keep the holders array size low, the _afterTokenTransfer() function checks if the sender has a balance of 0 after the transfer and then iterates through its entire length trying to find and remove the sender address. There is no break line and the function will iterate through the whole array on every transfer that has the sender remain with 0 tokens. Thus, users using the ERC20 transfer functionality to send all of their tokens will have their transactions start running out of gas due to hitting the block gas limit if the holders array grows enough.

But this is not all, the mint function will start hitting the block gas limit permanently. This is because it triggers the _afterTokenTransfer() function with address(0) as the sender. And since the 0 address cannot hold any tokens it will always trigger the loop to try and remove it from the array. This is exerpt from that ERC20 openzeppelin contract:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6edb6dd1ca43d05a762d84c688116b3327f5e490/contracts/token/ERC20/ERC20.sol#L251

    function _mint(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: mint to the zero address");

        _beforeTokenTransfer(address(0), account, amount);

        _totalSupply += amount;
        _balances[account] += amount;
        emit Transfer(address(0), account, amount);

        _afterTokenTransfer(address(0), account, amount);
    }

Note: The above link is to the commit of that contract in the openzeppelin repo, as it is a 3 years old I have to reference it this way.. It is the correct version 4.3.1, same as in package.json. You can of course check it locally.

The burn function will also revert if a user tries to burn all of their tokens as it also calls the _afterTokenTransfer() function with the users address and a after amount of 0.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6edb6dd1ca43d05a762d84c688116b3327f5e490/contracts/token/ERC20/ERC20.sol#L274

    function _burn(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: burn from the zero address");

        _beforeTokenTransfer(account, address(0), amount);

        uint256 accountBalance = _balances[account];
        require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
        unchecked {
            _balances[account] = accountBalance - amount;
        }
        _totalSupply -= amount;

        emit Transfer(account, address(0), amount);

        _afterTokenTransfer(account, address(0), amount);
    }

Tools Used

Manual review

Recommended Mitigation Steps

One way to mitigate would be checking if the receiving account has balance of 0 before iterating and removing it from the holders array as well. This will conserve ERC20 functionality and will prevent a malicios approved user from artificially inflating the size of the array, however it is still very much gas intesive. Perhaps a better solution would be to keep the indexes of holders in the holders array in a separate mapping say holdersIndexes, which would be of type address => uint. This will make access of the index of a holder constant time and will completely prevent this DoS/Gas Griefing attack from occuring.

Assessed type

DoS