YieldManager.sol

The YieldManager is the funnel through which all YieldInterface interactions are made. This is a good approach as it allows validation and logic to be persisted across any future implementations.

The issues found in the YieldManager mainly focus on better validation of the data being passed to interfaces.

[HIGH] The approach to disabling YieldInterface is inconsistent

If an interface is enabled, but then a problem requires it to be disabled (for security concerns, upgrade to functionality, etc.) then the interface would no longer be able to be interacted with via the YieldManager.

From our understanding of the codebase, this is expected to then only allow for tokens to be withdrawn as a means of extracting the tokens without further interaction. Since interfaces are locked to only receive requests from the YieldManager as a validation funnel, this would result in restricted access until the YieldInterface is once again enabled.

This works by using an internal validation check validAndHeldPosition, which would return empty positions for view calls and would prevent claim calls from being made at all. The withdraw functionality is still enabled, however this internally makes a claim against the position which would otherwise be prevented with its disabled state.

This poses a problem, as if there was a discovered exploit or issue in a YieldInterface then users would still be able to make claims during the withdraw call, even though all view calls show the position as empty or unknown.

In our opinion, one of two approaches should be taken:

1. Prevent claim being made during the withdraw flow if the YieldInterface has been disabled. This will persist the expected logic across all of the function calls.
2. Introduce an enum state approach for YieldInterface references, allowing for OPEN, NO_DEPOSIT, WITHDRAW_ONLY and LOCKED states. This introduces a tiered approach allowing for more granular control.
   • OPEN is the same as the current enabled state
   • NO_DEPOSIT prevents deposits while allowing for all other interactions
   • WITHDRAW_ONLY allows for liquidity positions to be withdrawn to the original owner, but does not claim inside the withdraw call. This could also remove the caller validation so that anyone can escape the ERC721 back to the original owner.
   • LOCKED prevents all interactions

Affected Code:

function withdraw(LPPosition calldata _lpPosition) external nonReentrant {
  // ..

  // Before we withdraw the LP position, we need to claim any outstanding fees
  _lpPosition.yieldInterface.claim(_lpPosition.data);

  // Withdraw the LP Position from the yield interface
  _lpPosition.yieldInterface.withdraw(_lpPosition.data, msg.sender);

  // ..
}

Recommended solution:

For the first option, we can just prevent the claim call in withdraw.

function withdraw(LPPosition calldata _lpPosition) external nonReentrant {
  // ..

  // Before we withdraw the LP position, we need to claim any outstanding fees
  // @audit If the interface has been disabled, then we need to prevent the claim from taking place. We have
  // already checked that the position is held and the owner is valid in a previous call.
  if (!validYieldInterfaces[_lpPosition.yieldInterface]) {
    _lpPosition.yieldInterface.claim(_lpPosition.data);
  }

  // Withdraw the LP Position from the yield interface
  _lpPosition.yieldInterface.withdraw(_lpPosition.data, msg.sender);

  // ..
}

For the second option, the enum can be defined in a linear order, with the most restrictive permission at the start. This will allow a modifier or internal validation check to compare the require role to the role state (State.OPEN >= State.WITHDRAW_ONLY).

enum State {
  LOCKED,
  WITHDRAW_ONLY,
  NO_DEPOSIT,
  OPEN
}

function withdraw(LPPosition calldata _lpPosition) external nonReentrant {
  // ..

  // @audit If the state is locked, then prevent any withdrawals
  if (hasState(State.LOCKED)) {
    revert WithdrawDisabled();
  }

  // Before we withdraw the LP position, we need to claim any outstanding fees
  // @audit We can only make a claim with the NO_DEPOSIT state or higher, otherwise we need to prevent the claim
  // from taking place.
  if (hasMinimumState(State.NO_DEPOSIT)) {
    _lpPosition.yieldInterface.claim(_lpPosition.data);
  }

  // Withdraw the LP Position from the yield interface
  _lpPosition.yieldInterface.withdraw(_lpPosition.data, msg.sender);

  // ..
}

[MEDIUM] Missing Access Control for Critical Functions

The claim function allows for claimants that are not approved to be able to call the actual claim function of the yield interface. Although this call will subsequently revert if there is no actual ETH generated from the claim call, it does still allow for the logic to be triggered on the external yield interface.

This could have negative effects as it is assumed in YieldInterface calls that validation has already been made before the claim function is called. There could be negative impact by being able to call claim, even when no ETH is returned.

Affected Code:

function claim(LPPosition calldata _lpPosition) public nonReentrant returns (uint recipientClaimed_, uint ownerClaimed_) {
  // Validate that the LP position is valid and held by the YieldManager
  if (!validAndHeldPosition(_lpPosition)) {
    return (recipientClaimed_, ownerClaimed_);
  }

  // Claim against the LP position
  uint claimed = _lpPosition.yieldInterface.claim(_lpPosition.data);

  // If we received ETH back from the claim, then add it to the recipient and track it
  if (claimed != 0) {
    // ..

    // Check if the caller is an approved claimant
    if (!isApprovedClaimant(_lpPosition, msg.sender)) {
      revert NotApprovedClaimant();
    }

    // ..
  }
}

Recommended solution:

By moving the claimant address validation further up the stack, we will ensure that no external claim call can be made and gas waste will be reduced if a non-claimant makes the call.

function claim(LPPosition calldata _lpPosition) public nonReentrant returns (uint recipientClaimed_, uint ownerClaimed_) {
  // Validate that the LP position is valid and held by the YieldManager
  if (!validAndHeldPosition(_lpPosition)) {
    return (recipientClaimed_, ownerClaimed_);
  }

  // Check if the caller is an approved claimant FIRST
  if (!isApprovedClaimant(_lpPosition, msg.sender)) {
    revert NotApprovedClaimant();
  }

  // Claim against the LP position
  uint claimed = _lpPosition.yieldInterface.claim(_lpPosition.data);

  // ..
}

[LOW] Constants should be moved to constant variables

For gas optimization and readability, inline constants should be moved to variables.

Affected Code:

// Validate the recipient share
if (_recipientShare == 0 || _recipientShare > 100) {
  revert InvalidRecipientShare();
}

// Validate the number of approved claimants to prevent griefing loops
if (_approvedClaimants.length > 10) {
  revert InvalidApprovedClaimants();
}

Recommended solution:

uint internal constant MIN_RECIPIENT_SHARE = 0;
uint internal constant MAX_RECIPIENT_SHARE = 100;

uint internal constant MAX_APPROVED_CLAIMANTS = 10;

// Validate the recipient share
if (_recipientShare == MIN_RECIPIENT_SHARE || _recipientShare > MAX_RECIPIENT_SHARE) {
  revert InvalidRecipientShare();
}

// Validate the number of approved claimants to prevent griefing loops
if (_approvedClaimants.length > MAX_APPROVED_CLAIMANTS) {
  revert InvalidApprovedClaimants();
}

[LOW] Deposit can optimize code by calling isValid

Two checks are being made that are already combined within an existing function. Since they both revert with the same selector, these can be merged.

// Ensure that the interface is valid
if (!validYieldInterfaces[_lpPosition.yieldInterface]) {
  revert InvalidYieldInterface();
}

// Validate the LP Position against the yield interface
if (!_lpPosition.yieldInterface.isValid(_lpPosition.data)) {
  revert InvalidYieldInterface();
}

// Ensure that the interface and position are valid
if (!isValid(_lpPosition)) {
  revert InvalidYieldInterface();
}