bug: `ExitFeeHookExample.sol` has a reentrancy problem

[JSeam2]

Is there an existing issue for this?

• I have looked through the existing issues

Which method was used to setup Scaffold-ETH 2 ?

git clone

Current Behavior

The reentrancy problem is introduced in the following line

            // Donates accrued fees back to LPs
            _vault.addLiquidity(
                AddLiquidityParams({
                    pool: pool,
                    to: msg.sender, // It would mint BPTs to router, but it's a donation so no BPT is minted
                    maxAmountsIn: accruedFees, // Donate all accrued fees back to the pool (i.e. to the LPs)
                    minBptAmountOut: 0, // Donation does not return BPTs, any number above 0 will revert
                    kind: AddLiquidityKind.DONATION,
                    userData: bytes("") // User data is not used by donation, so we can set it to an empty string
                })
            );
        }

Note that in Vault.sol, _removeLiquidity has the nonReentrant modifier.

However, the next section which routes the logic to the hook does not have reentrancy protection

        // AmountsOut can be changed by onAfterRemoveLiquidity if the hook charges fees or gives discounts.
        // Uses msg.sender as the Router (the contract that called the Vault).
        if (poolData.poolConfigBits.shouldCallAfterRemoveLiquidity()) {
            // `hooksContract` needed to fix stack too deep.
            IHooks hooksContract = _hooksContracts[params.pool];

            amountsOut = poolData.poolConfigBits.callAfterRemoveLiquidityHook(
                msg.sender,
                amountsOutScaled18,
                amountsOut,
                bptAmountIn,
                params,
                poolData,
                hooksContract
            );
        }

If a user uses the default example hooks unknowingly, the following attack can happen

• An attacker deploys a malicious contract that implements a callback function
• The attacker removes liquidity from a pool using the Vault
• When the Vault calls onAfterRemoveLiquidity on the hook:
• The hook calculates fees and calls _vault.addLiquidity()
• The Vault processes this addLiquidity operation
• Since the to parameter is the attacker's contract, it receives the callback
• _removeLiquidity has the reentrancy guard to prevent the pool from being drained. So a severe attack would be mitigated. However, given the reentrancy, the pool will be in an unbalanced state despite BPT tokens not being burned. Should there be other functions or contracts reliant on that pool's state, it might introduce other unexpected behaviors that could be exploited elsewhere.

Expected Behavior

Add a reentrancy guard for the example hooks, or make it explicit that the example hooks should not be used in production.

Steps To Reproduce

No response

Anything else?

No response