radevauditor - Potential underfunding in the `_fundPool` method due to ERC20 tokens that don't revert on over-withdrawals.

[sherlock-admin2]

radevauditor

high

Potential underfunding in the _fundPool method due to ERC20 tokens that don't revert on over-withdrawals.

Potential underfunding in the _fundPool method due to ERC20 tokens that don't revert on over-withdrawals.

Vulnerability Detail

If a user funds the pool with an amount they don't possess and the ERC20 token used does not revert on such over-withdrawals (instead, it transfers whatever amount the user has), the _fundPool function can complete its execution with less funds than expected. For example, if amountAfterFee is 50, but the user only has 20 tokens, and the ERC20 doesn't revert on over-withdrawals but sends the maximum it can (20 in this case), the pool is underfunded, but the function won't revert, leading to a discrepancy between the expected and the actual funded amount.

Proof of Concept (PoC)

Scenario:

Imagine a scenario where Alice wants to fund the pool using a particular ERC20 token, named "FaultyToken", which has a unique behavior: instead of reverting on over-withdrawals, it sends the maximum balance available.

Initial Setup:

1. FaultyToken balance of Alice: 20 tokens
2. Allowance given by Alice to the contract: 100 tokens
3. Alice attempts to fund the pool with: 50 tokens

Steps:

1. Starting the Process: Alice calls the _fundPool function, specifying she wants to deposit 50 tokens to the pool.

2. Fees Calculation: The contract calculates any fees. Let's assume there's a 10% fee. Therefore, feeAmount becomes 5 tokens, and amountAfterFee becomes 45 tokens.

3. Transferring Fee:

   • The contract tries to transfer the feeAmount (5 tokens) to the treasury.
   • Since Alice has 20 tokens, the FaultyToken transfers 5 tokens to the treasury without any issues.

4. Transferring to Strategy:

   • The contract then tries to transfer the amountAfterFee (45 tokens) to the strategy's address.
   • Here's where the fault comes in: Instead of reverting due to insufficient balance, FaultyToken transfers the maximum it can, which is the remaining 15 tokens in Alice's account, to the strategy.

5. Increasing Pool Amount: The contract calls _strategy.increasePoolAmount(amountAfterFee); thinking it transferred 45 tokens, but in reality, only 15 tokens were transferred.

6. Completion: The function completes without any errors, logs are emitted, and now there's a mismatch. The strategy believes it has 45 tokens more, but it only received 15 tokens.

Result:

The pool believes it has received 45 tokens from Alice when, in reality, it has only received 15 tokens. This underfunding can have various downstream implications like incorrect allocations, distributions, and other faulty behaviors based on this incorrect assumption of the pool's balance.

Impact

The pool can be underfunded, potentially leading to downstream logic issues, such as incorrect allocations, distributions, or other unintended behaviors, depending on the use cases of the pool and strategy.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/core/Allo.sol#L496-L520

/* ... */

_transferAmountFrom(
    _token,
    TransferData({
        from: msg.sender,
        to: address(_strategy),
        amount: amountAfterFee
    })
);
_strategy.increasePoolAmount(amountAfterFee);

/* ... */

Tool used

Manual Review

Recommendation

I Recommend these Possible Solutions:

1. Input Validation: Before executing the transfer, validate that the user has sufficient balance and has approved the contract to transfer the exact amountAfterFee.

   IERC20 token = IERC20(_token);
   require(token.balanceOf(msg.sender) >= amountAfterFee, "Insufficient balance");
   require(token.allowance(msg.sender, address(this)) >= amountAfterFee, "Allowance too low");

2. Standards Check: Only integrate with ERC20 tokens that behave according to the standard. An ERC20 token should revert on transfer or transferFrom if the specified amount is greater than the balance or allowance.

3. Strategy Verification: When incorporating new ERC20 tokens into your ecosystem, make sure to audit or verify their behavior to ensure they adhere to the expected standards. Consider maintaining a whitelist of approved tokens that have been verified to meet these criteria.

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, safeTransferFrom protects against this