Allo user is forced to supply more Eth than necessary but it's not credited back the remainder

The current implementation of Allo.sol doesn't allow to create and fund a pool with Native token where baseFee + amount is exactly msg.value - this forces the user to always transfer more msg.value than intended but doesn't credit back the excess. Usually supplying more msg.value than needed is considered a user mistake, however, in this case the user is required to do so but doesn't receive the excess back which is a valid loss of funds for the user & could also break integrations of other systems / contracts with the Allo pool creation.

Vulnerability Detail

In the function _createPool(...) we have the following check (displayed in the code snippet below). Assume that we want to create and fund a pool with the Native token for amount=X and a baseFee=Y - from the if statement below you can see that if baseFee + _amount >= msg.value we will have a revert, this implies that msg.value must always be greater than X + Y, therefore, for a successful creation of a pool the user must always overpay in msg.value, however, the excess of msg.value is never credited back to the user and causes an unnecessary loss of funds.

function _createPool(
        bytes32 _profileId,
        IStrategy _strategy,
        bytes memory _initStrategyData,
        address _token,
        uint256 _amount,
        Metadata memory _metadata,
        address[] memory _managers
    ) internal returns (uint256 poolId) {

        if (baseFee > 0) {
            // To prevent paying the baseFee from the Allo contract's balance
            // If _token is NATIVE, then baseFee + _amount should be >= than msg.value.
            // If _token is not NATIVE, then baseFee should be >= than msg.value.
            if ((_token == NATIVE && (baseFee + _amount >= msg.value)) || (_token != NATIVE && baseFee >= msg.value)) {
                revert NOT_ENOUGH_FUNDS();
            }
            _transferAmount(NATIVE, treasury, baseFee);
            emit BaseFeePaid(poolId, baseFee);
        }

        if (_amount > 0) {
            _fundPool(_amount, poolId, _strategy);
        }

        emit PoolCreated(poolId, _profileId, _strategy, _token, _amount, _metadata);
    }

Impact

Unnecessary loss of funds. This issue can make Allo difficult to integrate with.

Code Snippet

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

Tool used

Manual Review

Recommendation

Rework the if statement to:

if ((_token == NATIVE && (baseFee + _amount > msg.value)) || (_token != NATIVE && baseFee >= msg.value)) {
        revert NOT_ENOUGH_FUNDS();
  }

sherlock-audit / 2023-09-Gitcoin-judging