Althea Liquid Infrastructure - Babylen's results

Lines of code

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

Vulnerability details

Description

Transfering LiquidInfrastructureERC20 tokens to an approvedHolder with 0 balance adds them to a holders list in _beforeTokenTransfer:

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

Here in particular:

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

The holders array is used for reward token distribution in distribute function:

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

    uint i;
    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);
        }
    }
    nextDistributionRecipient = i;

As you can see, for each entry in the holders array their entitlement is calculated and transferred according to holder's balance:

uint256 entitlement = erc20EntitlementPerUnit[j] * this.balanceOf(recipient);

erc20EntitlementPerUnit is calculated from supply and rewards balance when distribution begins (in _beginDistribution) and does not take into account the holders array length.

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

The problem becomes apparent when we consider 0 amount token transfers, which are completely valid in the ERC20 implementation used by the LiquidInfrastructureERC20 contract. LiquidInfrastructureERC20 itself doesn't prohibit such transfers either. If 0 tokens are send to an approved holder that doesn't hold any tokens, _beforeTokenTransfer function will add their address to the holders array even though their balance will still be 0. This can be repeated multiple times to gain multiple entries (and entitlements) for one holder. Then, if a balance is transferred to this holder, they will have multiple full entries for rewards distribution. This not only prevents the distributeToAllHolders() function from ever succeeding with a non-0 balance of reward tokens (due to it trying to send full reward to one holder multiple times, eventually running out of tokens), but also, once distribution starts, allows the holder in question to syphon off rewards from any holder that entered after them, as well as render the contract effectively unusable and tokens untransferable due to the fact that distribution cannot end.

Impact

Consider the following scenario:

Alice is an approved holder and is also malicious. Before Alice holds any tokens (or after burning/transfering away her balance) she can initiate many 0 transfers to her account and fill the holders array with duplicate entries of her address. Source of transfers doesn't matter here as the contract never checks if a sender is an approved holder.

If any holders appear after Alice and the contract gets (or already was) funded by reward tokens, Alice will be able to syphon new holders' rewards by using the distribute function (demonstrated in the POC).

Alice can monitor any LiquidInfrastructureERC20 she is an approved holder on and frontrun any new holders' appearance (such as a new approved holder buying tokens from a marketplace or from another holder directly - maybe even from Alice herself) with 0 transfers to her account to add her duplicate entries before the new holder entry. If contract is funded with rewards and distribution is possible, Alice can immediately call distribute, sandwiching the new holder's transaction, and receive all of the tokens the new holder is entitled to for herself. If contract is funded but distribution is not possible, Alice can simply wait for MinDistributionPeriod as there isn't another way to withdraw rewards from the contract.

Alice can also choose to run the attack on an unfunded contract with hopes of it being unnoticed, allowing her to steal the rewards at a later time (similarly, when frontrunning a new holder in the previous examples, Alice can choose to not distribute right away and wait for more victims and/or rewards). This would rely on nobody realizing that an attack is being prepared, which might not be unlikely considering that the holders array is private and the only clues would be the 0 transfers themselves, which can be further obfuscated by doing them as internal transactions.

Additionally, this attack poisons the contract unless owner removes Alice from approved holders.

Another effect of this is that distribution, once started, will not be able to end and the holders won't be able to transfer their tokens unless the contract gets funded by more reward tokens and the owner removes Alice from approved holders.

function _beforeTokenTransfer(
    address from,
    address to,
    uint256 amount
) internal virtual override {
    require(!LockedForDistribution, "distribution in progress");

Any non-holder is also able to poison the contract in this a way by finding an approved holder address with 0 balance and spamming 0 transfers to them.

Attacks described above rely on one of the two assumptions to be true: a) The attack preparation stage (0 transfers) are unnoticed/not believed to be malicious or b) The attacker is able to sandwich a transaction that appends a new holder in a contract that has rewards already deposited and is ready for distribution

If an attacker manages to gain access to 3 or more approved holder accounts (or three or more malicious holders cooperate), an invisible attack becomes possible:

1. Alice, Bob and Charlie are holders. Alice and Bob hold 50 tokens each, Charlie holds 0

2. David is a holder added after Alice and Bob. David has 100 tokens. holders looks like this: [Alice, Bob, David]

3. The attacker waits for the contract to receive reward tokens and for the distribution period to elapse. No holders have done anything suspicious so far

4. Attacker initiated 0 transfers to Charlie's address. This appends Charlie to the end of the holders array.

5. Alice transfers her 50 tokens to Bob. This removes Alice's entry from holders and replaces it with Charlie's that's at the tail of the array:

   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 we can see, when a holder has 0 balance after making a transfer, their entry is replaced with the last, then the last is removed.

6. Bob sends their 100 tokens to Charlie. This removes Bob from holders, and puts another Charlie entry in their place. Now holders looks like this: [Charlie, Charlie, David, Charlie, ..., Charlie]
7. Attacked calls distribute(2) and Charlie receive two times the amount of rewards that 100 tokens entitle them to, and David receives nothing.

Steps 4-7 can be done in a single block, frontrunning anyone else's attempts to distribute if necessary. As the attacker won't be able to transfer out their LiquidInfrastructureERC20 tokens after running the attack, the profitability of the attack depends on the cost of LiquidInfrastructureERC20 tokens in relation to the cost of the reward token. An attacker can increase their profitability by using more holder accounts (3 holder accounts = 2x reward entitlement, 4 holder accounts = 3x, etc).

Proof of Concept

1. Clone the repo

2. Get liquidERC20attack.ts and TestERC20A.sol from the gist: https://gist.github.com/CrystalPony/2e31441d311371f208e663cf60d3e2ef

3. Replace TestERC20A.sol in /contracts Note that this has no relation to the vulnerability itself, I've simply removed minting to the default ethers addresses from the constructor to make the testing more straight-forward (I could have used any basic ERC20 instead).

4. Add liquidERC20attack.ts to /test

5. run npm run test

The POC demonstrates that a malicious holder is able to steal the rewards from holders that join after them by exploiting 0 balance transfers before the new holders receive their balance, and calling distribute after.

Tools Used

hardhat, hardhat-ethers, chai, Ethereum-waffle

Recommended Mitigation Steps

A straight-forward solution would be to check if holder is already in holders before pushing to the holders array. A more gas-efficient solutions could be to check the transfer amount and disallow 0 transfers (or not append to holders).

Assessed type

Token-Transfer