Althea Liquid Infrastructure - 0xJoyBoy03's results

Lines of code

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

Vulnerability details

Impact

There is a potential DoS attack in the _afterTokenTransfer function which will make the protocol break and the owner can't mint tokens again. As we know "When a function call gets a gas error The entire transaction, including any changes made by previous function calls within the transaction, is reverted. This means that all state changes made up to that point are undone"

Proof of Concept

in this snippet code in the _beforeTokenTransfer function:

//existing codes...

// @note: Alice and users that Alice approved can burn Alice's tokens to push address(0) to `holders`
 @>       bool exists = (this.balanceOf(to) != 0);
        // @audit-High : add this condition to prevent DOS attack in _afterTokenTransfer : if(!exists && to != address(0))
        if (!exists) {
            holders.push(to);
        }

every time a user wants to burn its tokens, the zero address will be pushed to holders because the exists becomes false because the balance of zero address for the token is 0 thanks to the _burn function which decreased the balance of the from but not increased the balance of zero address.

Here is the step-by-step scenario of how a malicious user can make a DoS attack:

1. after the deployment, the owner mints tokens for some users (for example 10,000).

2. a malicious one burns its tokens one by one, unit by unit to push more zero addresses to the holders array. He can also do that with an approved malicious contract by the user to keep calling the burnFrom(_maliciousUser, 1) function. this is possible because the balance of zero address is always will be 0 and it will enter the if statement:

// `exists` is equal to false
if(!exists){
    holders.push(to);
}

3. we might see 10,000 zero addresses in holders that aren't the actual holders. however, lots of users might burn their tokens, and for everyone who burns tokens, a zero address will always be pushed so 10,000 is just a small number.

4. in the _afterTokenTransfer function :

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

of course, the goal of the function is that every holder that leads its balance to zero of this token contract will be removed from the holders array. when the owner the next time after the deployment wants to mint tokens again via the mint function, the whole process of minting will be reverted due to the gas error. because:

//ERC20.sol
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);
    }

The _mint function will call the _afterTokenTransfer and pass the zero address as from then from will enter the loop in _afterTokenTransfer and pop up every zero address that exists in holders but due to the large number of them,

it'll get reverted...

Tools Used

Manual Review

Recommended Mitigation Steps

Don't let the zero address be pushed to the holders array.

     //existing codes...
     if (!exists && to != address(0)) {
            holders.push(to);
        }

Assessed type

DoS

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

Summary: everytime the approved users burn their tokens, the zero address will be pushed to the holders array.

Detail: inside the _beforeTokenTransfer function:

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();
        }
        // @note: Alice and users that Alice approved can burn Alice's tokens to push address(0) to `holders`
        bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }
    }

as you see if approved users burn their tokens, the zero address will be pushed to the holders array cause the zero address's balance is always 0. this is because, in the ERC20.sol in the _burn function, the function reduces the balance of the from but doesn't increase the balance of the zero address. so It will cause to exists variable to always be true if to is a zero address.

Impact: Holders should be actual holders not a bunch of zero addresses. this is just breaking the functionality of the protocol.

Recommendation:

add another condition in the if statement to don't push zero addresses:

//existing codes...

+    if(!exists && to != address(0))

//existing codes...

Lines of code

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

Vulnerability details

Impact

There is a scenario in which a Malicious user can delay the revenue for another MinDistributionPeriod. Holders earn revenue from withdrawERC20s. The protocol obliged to aggregate all the withdrawERC20s tokens of all ManagedNFTs into LiquidInfrastructureERC20 and send them to holders. but a Malicious user can delay some of the lists of withdrawERC20s to another MinDistributionPeriod. This will lead to more unhappy customers and a weak protocol because the Attacker can freeze the revenues of holders for another MinDistributionPeriod.

Proof of Concept

Suppose we have these assumptions:

• MinDistributionPeriod = 30 days (assume X in block number)
• ManagedNFTs.length = 5
• withdrawERC20s (or distributableERC20s) = [Dai, USDT, USDC]

Here is the step-by-step scenario:

1. A malicious user can call the withdrawFromManagedNFTs(2) function in the X - 1 block number. the result is that all of the withdrawERC20s tokens in the first two ManagedNFTs are sent to the LiquidInfrastructureERC20 and it will charged. note that the nextWithdrawal variable is equal to 2.

2. After that, he will call the distributeToAllHolders() function to distribute the first two assets of ManagedNFTs to holders and end the distribution. this will lead to setting LastDitribution to the current block number and it'll kind of reset the MinDistributionPeriod

3. Holders must wait for another MinDistributionPeriod (30 days) to receive their shares from other assets of ManagedNFTs. their assets will freeze for another 30 days.

4. Griefing is Done!!!

@Note: This scenario can be worse if the owner pops up one of the ManagedNFTs with the releaseManagedNFT function. in our example, if the owner unintentionally pops up the first or second ManagedNFTs array, based on the protocol, the last member of the ManagedNFTs will replaced with the first or second member. then recalling the withdrawFromManagedNFTs function will query the ManagedNFTs starting from 2 cause the nextWithdrawal is 2 and the replaced member of ManagedNFTs should wait for another time.

Tools Used

Manual Review

Recommended Mitigation Steps

Make the withdrawFromManagedNFTs function internal to call it just via the withdrawFromAllManagedNFTs function or Add an onlyOwner modifier.

+ function withdrawFromManagedNFTs(uint256 numWithdrawals) public onlyOwner {

       //existing codes...

}

Assessed type

Access Control