Description

At the end of the withdraw function, the tokens are transferred to the user:

// Transfer to user
IERC20 underlying = IERC20(underlyingTokenAddresses[_index]);
underlying.approve(msg.sender, underlyingAmount);
underlying.transfer(msg.sender, underlyingAmount);

However, because the transfer is done with transfer and not transferFrom or safeTransferFrom, the approval to the sender is not spent. Even after the payment, the user can still withdraw underlyingAmount of the token by calling transferFrom themselves.

Impact

Definitive has the ability to withdraw tokens into the staking manager; the withdrawTo function is guarded with onlyWhitelisted. Thus, although no explicit functionality of the staking manager will leave the contract holding any funds, Definitive is allowed to perform such withdrawals and cause funds to be left in the staking manager contract.

Further, future functionality may include the staking manager taking custody of tokens (such as fees) as well.

In these cases, the extra approval will allow any user to steal rewards or future fees held by the staking manager. This could also be performed maliciously by Definitive. For instance, those with the ROLE_DEFINITIVE on the underlying strategy might be able to drain the contract by

1. depositing and withdrawing funds to increase unspent approval on the token,

2. calling withdrawTo on the underlying vault to withdraw funds into the

   staking manager, and

3. calling transferFrom on the underlying token into their own account.

Recommendations

Remove the unnecessary call to underlying.approve.

Remediation

This issue has been acknowledged by Rainmaker, and a fix was implemented in commit 46249703↗.