Allo::_fundPool()

Allo::_fundPool() - Although the protocol allows ALL ERC20's, the pool funding functionality doesn't currently accommodate token types which can increase/decrease received value during transfers.

This includes both fee-on-transfer tokens and rebasing tokens, however, this finding focuses only on fee-on-transfer tokens, but arguably the recommended handling of rebasing tokens should be similar.

Vulnerability Detail

The protocol applies a fee during execution of the internal _fundPool() function, but this fee is not related to fee-on-transfer tokens and completely independent. The function handles this correctly for native token(ETH) as well as for most normal/standard ERC20 tokens.

But it fails to take into account the additional fees from fee-on-transfer tokens, and the only existing method inside the function that COULD have checked this, but by default doesn't, is the below method, which I assume is there for when external projects build their protocols on top of the Allo protocol, so they can add their custom implementation logic for _beforeIncreasePoolAmount and _afterIncreasePoolAmount hooks.

    function increasePoolAmount(uint256 _amount) external override onlyAllo {
        _beforeIncreasePoolAmount(_amount);     
        poolAmount += _amount;
        _afterIncreasePoolAmount(_amount);
    }

Neither does the protocol's implementation of the safeERC20's safeTransferFrom library help to mitigate this issue, as those standard library functions dont check whether received token amount is equal to the value for function parameter amount.

Impact

So since the _fundPool() function doesn't currently seem to handle the additional fees from fee-on-transfer tokens, the effects of this would be:

potential revert in the best case scenarios
internal accounting not in sync with actual token balances.

If the pool implements fees, then the token amount received in treasury will be less than the feeAmount parameter value on L513:

_transferAmountFrom(_token, TransferData({from: msg.sender, to: treasury, amount: feeAmount}));

And same for actual token amount received in strategy contract, will be less than value of amountAfterFee parameter on L516:

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

So the internal accounting balances for treasury and address(_strategy) will be out of sync with their actual token balances.

And when the strategy contract distributes the funds:

In terms of all the other strategy types, this issue applies to all of them, but I will use one example:

DirectGrantsSimpleStrategy::_distributeUpcomingMilestone(), the recipient will always receive less tokens than amount transferred. State variable poolAmount is correctly accounted, but recipient.recipientAddress will receive pool.token amount < amount:

        poolAmount -= amount;
        _transferAmount(pool.token, recipient.recipientAddress, amount);

Code Snippet

https://github.com/allo-protocol/allo-v2/blob/851571c27df5c16f6586ece2a1cb6fd0acf04ec9/contracts/core/Allo.sol#L502-L520

Tool used

VSC. Manual Review

Recommendation

Since the protocol allows ALL ERC20 token types, as per the README.md, whitelisting is not a solution, so the only other solution is to implement balanceBefore and balanceAfter for the relevant functions where transfer of fee-on-transfer tokens is performed. Then the difference of the two balances, i.e. balanceAfter - balanceBefore will be the correct & actual token amount received, which will take into account the fees deducted by the fee-on-transfer tokens during transfers.

Duplicate of #19

sherlock-audit / 2023-09-Gitcoin-judging