Incorrectly Implemented Admin Fee Donation Function Disrupts Pool Accounting

[hats-bug-reporter[bot]]

Github username: @00xWizard Twitter username: 00xWizard Submission hash (on-chain): 0x05ef4795fa3e8bbca00768ddff5d4afc40d88e24dc5dba3448686d27c124dda8 Severity: high

Description: Description

The donate_admin_fees function doesn't donate or transfer any fees. Instead, it updates the contract's internal balances array to reflect the total balance of each token held by the contract. This implementation could lead to incorrect accounting within the pool, potentially affecting operations that rely on these balance values.

Attack Scenario\

1. The pool has 1000 USDC of user-provided liquidity.
2. Over time, the pool has accumulated 50 USDC in admin fees.
3. The balances array for USDC shows 1000 USDC (only user liquidity).
4. The pool now treats 1050 USDC as the total liquidity.
5. Liquidity providers who deposit after this point will be diluted, as they're buying into a share of the pool that includes the admin fees.
6. Users withdrawing liquidity will receive a proportion of the admin fees they're not entitled to.

also in addition

1. An owner could call this function strategically before making a large deposit, effectively "buying" a share of the admin fees at no cost.
2. Vice versa, they could call it before a large withdrawal, extracting more value than they're entitled to.

The function incorrectly assumes that updating the balances array to the total contract balance is equivalent to "donating" admin fees. In reality, this action merges distinct accounting categories (user liquidity and admin fees) that should remain separate.

Attachments

1. Proof of Concept (PoC) File

   function donate_admin_fees() external onlyOwner {
       for (uint256 i = 0; i < N_COINS; i++) {
           if (coins[i] == ROSE_ADDRESS) {
               balances[i] = address(this).balance;
           } else {
               balances[i] = IERC20(coins[i]).balanceOf(address(this));
           }
       }
       emit DonateAdminFees();
   }

2. Revised Code File (Optional)

uint256[N_COINS] private adminFees;
uint256 public totalSupply; // Total supply of LP tokens

++Rest of the Code++

function donate_admin_fees() external onlyOwner {
    for (uint256 i = 0; i < N_COINS; i++) {
        uint256 currentBalance;
        if (coins[i] == ROSE_ADDRESS) {
            currentBalance = address(this).balance;
        } else {
            currentBalance = IERC20(coins[i]).balanceOf(address(this));
        }

        uint256 fee = currentBalance - balances[i];
        if (fee > 0) {
            adminFees[i] += fee;
            balances[i] = currentBalance;
        }
    }

    if (totalSupply > 0) {
        for (uint256 i = 0; i < N_COINS; i++) {
            if (adminFees[i] > 0) {
                balances[i] += adminFees[i];
                emit AdminFeeDonated(i, adminFees[i]);
                adminFees[i] = 0;
            }
        }
    }

    emit DonateAdminFees();
}

event AdminFeeDonated(uint256 indexed coinIndex, uint256 amount);

the fix helps with the following:

1. correctly identifies and separates admin fees from the main pool balances.
2. it "donates" the admin fees by adding them to the pool's liquidity, which benefits all liquidity providers proportionally.
3. maintains accurate accounting of the pool's balances.

[omega-audits]

It is not clear at all why the following claim would be true:

Users withdrawing liquidity will receive a proportion of the admin fees they're not entitled to.

For two reasons:

• you do not show all how future lp providers are "recieving a portion of the admin fees"
• even if they did, it is not clear why they would "not be entitled" to these fees

btw, I think your "fix" is broken - if balances[i] = 100 and currentBalance is 110, it will set balances[i] to 120.