Twyne Incremental Review

A missing validation could be used to pull funds from dangling authorization.

The updated functionality of teleport() can be used to migrate a position from a subaccount of the borrower.

237:     function teleport(uint toDeposit, uint toBorrow, address subAccount) external onlyBorrowerAndNotExtLiquidated whenNotPaused nonReentrant {
238:         createVaultSnapshot();
239: 
240:         totalAssetsDepositedOrReserved += toDeposit;
241:         _handleExcessCredit(_invariantCollateralAmount());
242: 
243:         if (toBorrow == type(uint).max) {
244:             toBorrow = IEVault(targetVault).debtOf(subAccount);
245:         }
246: 
247:         IEVC.BatchItem[] memory items = new IEVC.BatchItem[](3);
248:         items[0] = IEVC.BatchItem({
249:             targetContract: asset(),
250:             onBehalfOfAccount: address(this),
251:             value: 0,
252:             data: abi.encodeCall(IERC20.transferFrom, (subAccount, address(this), toDeposit)) // needs allowance
253:         });

The problem is the absence of a validation that ties the subaccount to the current borrower, leading to a transferFrom() action from an arbitrary account.

Usually, there should not be any active allowance from third-party accounts, as collateral vaults are per borrower. However, since vaults can be liquidated, the borrower may shift to the new liquidator, enabling the new borrower to access the old borrower's funds.

High. New vault owners can siphon funds from a previous borrower.

Requirements:

• Collateral vault gets liquidated
• Excess approval exists
• Previous vault owner contains a non-zero amount of the collateral token
• Funds can ONLY be pulled from the previous vault owner

Validate subaccount is an actual subaccount of the current borrower.

Fixed in commit 5945b31. To clarify, the proper implementation already existed in the MockCollateralVault contract.

The LeverageOperator contract executes ERC20 operations over arbitrary tokens without the SafeERC20 wrapper, potentially causing incompatibility issues.

• LeverageOperator.sol#L156
• LeverageOperator.sol#L195

Low.

Use safeTransfer() and forceApprove().

fixed in PR#36.

The contracts referenced in the productionSetup() function are empty accounts in Ethereum.

TwyneDeployEulerIntegration.s.sol#L280-L284.

280:         } else if (block.chainid == 1) {
281:             oracleRouterFactory = 0x72735e5dd42EDc979c600766532eA704842CfB7b;
282:             evc = EthereumVaultConnector(payable(0xC36aED7b7816aA21B660a33a637a8f9B9B70ad6c));
283:             factory = GenericFactory(0xd5e966dB359f1cB2A01280fCCBEB839Ac572CE35);
284:             protocolConfig = ProtocolConfig(0x3b68711EF6c1988c96CBD32d929b76cB09b579Ea);

Informational.

Ensure to deploy these contracts properly before executing the script.

Acknowledged. These addresses are just placeholders. The deployment process involves executing an evk-periphery script which provides these addresses.

The initialCollateralBalance variable is written but never read.

LeverageOperator.sol#L40.

Informational.

Remove the variable.

fixed in PR#36.

The mechanism provided by flashloanLock does not enforce any lock.

The flashloanLock is toggled before executing the flashloan in executeLeverage(). The implementation doesn't provide any actual locking benefits because:

• This doesn't work as a reentrancy guard: the function can be re-entered while the flag is on.
• onMorphoFlashLoan() cannot be executed by anything other than the LeverageOperator itself, cause the caller must be Morpho, and Morpho only calls the flashloan initiator.

Informational. Gas savings.

Change the semantics of this lock to be a normal reentrancy guard in the executeLeverage() function or remove the flashloanLock variable.

flashloanLock variable removed in PR#36. Added nonreentrant modifier for extra safety, even if it is not necessarily required.

The function depositUnderlyingToIntermediateVault() takes an intermediateVault parameter and approves euler asset on it.

However this intermediateVault is not enforced to be a legit vault deployed by the factory. This allows any user to approve a malicious contract.

While the contract is not supposed to hold funds, it is advised to check the intermediateVault against the Vault manager.

Informational.

Ensure the intermediateVault is a legit vault against the Vault Manager and/or implement the Gas finding recommended which will remove the approval().

Fixed in PR#36.

The EulerWrapper contract enables a zapper functionality to deposit in Twyne.

Both functions have the callThroughEVC modifier, which will re-route the call through the EVC. This modifier shouldn't be needed as the implementation doesn't strictly require it. Additionally, note that each call to EVK vaults would need to be re-wrapped in the EVC that corresponds to each vault (which might be Euler's or Twyne's).

Additionally, the pull token → approve → deposit cycle can leverage the skim functionality and instead do:

1. transfer tokens from the caller to collateral
2. call skim() on the collateral with the intermediate vault as the recipient
3. call skim() on the intermediate vault with the user as the recipient

Gas savings.

Consider applying the suggested modifications.

fixed in PR#36.