BuyBackManager.sol

The BuyBackManager is a new manager that will attribute funds earned from the Flaunch tokens and deposit them into the BidWall position of a Flaunch token,

[MED] Potential MEV attack vector

As the routeBuyBack call is public, it will allow an educated token holder to monitor this and move the manager funds to the BidWall before selling at a better price.

This could be avoided by either protecting the function call or putting in place a programmatic approach (an offchain keeper, or Notifier subscriber) to ensure that the buy backs are made in smaller or more protected chunks.

[MED] Role removal will break existing groups

The BidWall ensures that the deposit function can only be called by a CA that has the POSITION_MANAGER role:

/**
  * Ensures that only a {PositionManager} can call the function.
  */
modifier onlyPositionManager {
    if (!hasRole(ProtocolRoles.POSITION_MANAGER, msg.sender)) revert NotPositionManager();
    _;
}

This means that as part of the deployment pattern, the manager implementation must always hold this role. This is enforced in two places via the _validatePoolKey function:

1. During the _initialize call, when the manager is deployed
2. During the unlockCallback when the buyBack is being routed

The second function call does not actually require the full validation of the PoolKey, as it only actually needs to know the _nativeIsZero boolean value. This could instead be determined by the currency0 slot directly, as the PoolKey is immutable and already set.

The issue with the current approach is that if the role was removed from the implementation through accidental centralised interactions then all existing manager deployments would no longer be able to perform buy backs resulting in funds being locked inside the respective managers. This could be rectified by just adding the role back in, but better to not have this potential issue.

[LOW] BidWall disabled state is not respected

If the BidWall has been disabled on the PoolKey that is specified, then ETH funds will still be transferred to the BidWall and create a position when enough funds are consolidated. However, on the PositionManager, an additional check is made to BidWall.isBidWallEnabled to detect if the BidWall is disabled for that token and, if it is, routes fees to the MemecoinTreasury instead.

The BidWall will still work if it's disabled, but it's not clear if this is the desired functionality.

[LOW] Transfer call to flETH not validated

The buyBack function makes external deposit and transfer calls to the flETH contract without proper validation of return values. It would be safer and prevent loss of funds to implement the already referenced Currency.transfer function, or use the SafeTransferLib for the transfer function.

IFLETH(Currency.unwrap(flETH)).deposit{value: msg.value}(0);
flETH.transfer(address(_buyBackPoolKey.hooks), msg.value);

[INFO] Additional BidWall Deposit event data

The current deposit event only has an amount of ETH that is added to the BidWall. This event would benefit from additionally having context of the PoolKey that is having deposit called against it.

[INFO] Better balances support with no parameter

For better cross-manager support, it would be recommended to define a balances function that passes the msg.sender, in addition to one that allows the _recipient to be specified.

[INFO] Prioritise msg.value check in buyback function call

In the unlikely event that buyBack is called without ETH, it would be more beneficial to put the msg.value check first as the others make external (more gas intensive) calls.

// If we have not been passed any ETH in the call, then we need to revert
if (msg.value == 0) {
    revert InvalidImplementation();
}