Althea Liquid Infrastructure - turvy_fuzz's results

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L130 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L142-L145 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L171 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L216 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/main/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L234

Vulnerability details

Vulnerability details

Let's understand the code logic of the override _beforeTokenTransfer() which:

Implement locks on transfers when locked for distribution Enforces receivers to be approved Adds approved receivers to the list of holders when needed

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

The issue is in the addition of approved receivers to the list of holders. The _beforeTokenTransfer() adds approved receivers to the list of holders with the below code logic

bool exists = (this.balanceOf(to) != 0);
        if (!exists) {
            holders.push(to);
        }

It checks if the balance of the receiver was zero BEFORE the token transfer and then adds the address to the holders array if so, however, It doesn't check if the balance of the receiver is still zero AFTER the token transfer which is possible when a user specifies a 0 amount which will pass since no code in the contract prevents this. This means when the user transfers the 0 amount again to the same receiver, the approved receiver's balance would still be zero and the same address will be pushed to the holder's array again, and so on. Increasing the array at will, with duplicates

Note, that another issue of missing approval checks made only to the receiver and not the sender enhances this bug likelihood, as anyone can perform this without any KYC or approval and cannot be restricted (right now, even from addresses that have been disapproved can still hold tokens and perform transfers with them if no longer approved).

Allowing anybody to break the protocol due to the way the holders array and 'from' address are handled in _beforeTokenTransfer

Impact

There are 4 separate impacts of this bug:

1. Approved holders can take advantage of this to drain the protocol, by adding their address multiple times to the holders array before a distribution and then calling distribute() to receive rewards for as many times as they want.

2. _afterTokenTransfer() will fail for holders that need to move out all their tokens, as the loop operation will end up running out of gas. This also means that the holders array will permanently never be able to decrease.

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

3. Attackers can grief any APPROVED holder whose balance was zero, by adding the specific holder's address multiple times into the array and forcing the owner to disapprove the honest holder in order not to break the distribution and allow distribution to continue, preventing them from receiving any more of the underlying ERC20.

4. And lastly, attackers can break the entire protocol by transferring zero amount to multiple APPROVED holders addresses with an amount balance of zero for a significant amount of times, and then begin a distribution after performing this, making it unable to end distribution and therefore locking the protocol

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

Proof of Concept

Consider the following exploit scenario:

• Alice is an approved holder and owns 100 tokens.
• Then, distribution is about to begin and _isPastMinDistributionPeriod() will return true
• Alice then sells all her tokens to an approved holder or marketplace setting Alice's balance to zero and removing her from the holders' array.
• However, Alice is now able to call transfer to her address with zero amount multiple times and effectively add her address back to the holders array as many times as she wants and then lastly trigger a final transfer of her initial 100 tokens by buying it back.
• Now, Alice calls distribute() which ends up transferring rewards to Alice as many times as her address was added to the holders' array since there is no check for duplicate addresses that has already received rewards.

for (i = nextDistributionRecipient; i < limit; i++) {
@>          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);
            }
        }

Recommended Mitigation Steps

The fix for the root cause is quite straightforward:

Check that the amount to be sent is > 0 before adding to the holders array, here is the code change:

bool exists = (this.balanceOf(to) != 0);
-       if (!exists) {
+       if (!exists && amount > 0) {
            holders.push(to);
        }

Now that is clear, additionally, I might recommend using the EnumerableSet library from openzeppelin for the holders array and/or also check for duplicate addresses that have already received rewards during distribution. Lastly, maybe consider adding a check on the from address too either on the _beforeTokenTransfer() or _afterTokenTransfer().

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Calling setDistributableERC20s when a distribution has already begun will break the protocol distribution and accounting

Proof of Concept

Entitlement to be distributed for each distributable token is calculated based on the balance the protocol has for that token to avoid a case of insufficient amount failures. as seen below: _beginDistribution()

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

This also means that erc20EntitlementPerUnit is directly matched to the distributableERC20s array including its indexes. Most importantly, this calculation is done only at the beginning of the distribution. distribute()

...
@> if (!LockedForDistribution) {
            require(
                _isPastMinDistributionPeriod(),
                "MinDistributionPeriod not met"
            );
            _beginDistribution();
    }
...

Now, the owner can change this list of distributable tokens using the setDistributableERC20s():

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

However, this function can still execute even when a distribution has begun. now when someone calls distribute after the update, the erc20EntitlementPerUnit still be based on the former distributableERC20s values. Due to this mismatch, the transfer operations in the distribution will break depending on the newly changed tokens, and its balances might revert, return false, or pass erroneously all leading to an incorrect accounting state

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

Lastly, aside from the owner not knowing about this, an attacker can also cause this when the distribution hasn't begun as long as the owner calls setDistributableERC20s() when the minimum distribution period has passed, as the attacker can see this in a mempool and begin distribution ahead of the transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

Prevent the setDistributableERC20s() function from executing when the distribution has already begun. fix:

function setDistributableERC20s(
        address[] memory _distributableERC20s
    ) public onlyOwner {
+       require(!LockedForDistribution, "distribution in progress");
        distributableERC20s = _distributableERC20s;
    }

Assessed type

DoS