Skip to content

Conversation

@hoytech
Copy link
Contributor

@hoytech hoytech commented Dec 14, 2024

No description provided.

Comment on lines 31 to 34
require(locked == 0, Locked());
locked = 1;
_;
locked = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we okay with tstore / tload? otherwise, 1 and 2 instead of 0 and 1 is usually used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The locked is in the same storage slot as the reserve{0,1} variables, so the 1<>2 optimisation won't help (we're not getting refunds anyway). I think we'd probably be OK with transient storage, although maybe we want to leave the door open to older networks without this feature. I'll measure and see if it helps much gas-wise.

Comment on lines 148 to 161
function withdrawAssets(address vault, uint256 amount, address to) internal {
uint256 balance = myBalance(vault);

if (balance > 0) {
uint256 avail = amount < balance ? amount : balance;
IEVC(evc).call(vault, myAccount, 0, abi.encodeCall(IERC4626.withdraw, (avail, to, myAccount)));
amount -= avail;
}

if (amount > 0) {
IEVC(evc).enableController(myAccount, vault);
IEVC(evc).call(vault, myAccount, 0, abi.encodeCall(IBorrowing.borrow, (amount, to)));
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: the withdraw could revert because there's not enough cash in the pool to withdraw.
But withdraw + borrow draw from the same pooled cash, so there's no real alternative here and one might as well always try to withdraw the entire balance regardless of the cash, as we do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's best to just try it and fail if cash is not sufficient since there isn't really anything else to do. Hopefully the quoting will take the pooled cash amounts into account and people won't attempt to do such swaps. It might be worthwhile try/catching this and failing with a nicer error message though.

Comment on lines 194 to 198
require(quote <= (asset0IsInput ? reserve1 : reserve0), InsufficientReserves());
require(
quote <= IERC20(asset0IsInput ? asset1 : asset0).balanceOf(asset0IsInput ? vault1 : vault0),
InsufficientCash()
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also revert if amountIn would be higher than the supply cap? Note that we'd need to take into account repaid debt for the effective deposit amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's also the same problem with borrow caps.

Comment on lines 194 to 198
require(quote <= (asset0IsInput ? reserve1 : reserve0), InsufficientReserves());
require(
quote <= IERC20(asset0IsInput ? asset1 : asset0).balanceOf(asset0IsInput ? vault1 : vault0),
InsufficientCash()
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one could ask if we should revert if the swap ends up putting the account into an unhealthy state (if they are not yet unhealthy) but I consider that a bug that needs to be fixed in the swap itself.
It shouldn't be possible to attempt to perform such a swap because the debt limits (if they are reasonable) combined with the initial deposit define a max LTV that shouldn't be crossable by any swaps.
Would you agree?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With "the account" you mean myAccount?
Because any withdraw or borrow should trigger (deferred or not) a status check on the myAccount anyway, so it's impossible that myAccount ends up liquidable.

It's true that it could end up on the verge of the liquidation with some accrued interest accrued in the next blocks.

A possible solution to mitigate the issue could be to have a "buffer" HF threshold to be respected after a swap that could be configured when the Maglev is deployed or at any time (depending on how much you want to be flexible).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sorry, I forgot we were inside the _computeQuote where it's not taking in consideration the HF of myAccount after the swap.

There's not a real standard applied on top of a lending protocol where you also need to take in consideration the HF of the account (the LP) and all the boundaries (min debt, supply/debt limits and so on) but I would say that this function should be seen as a "simulation" of the swap and should revert like if the swap would revert.

But at this point, wouldn't it make more sense to really simulate the swap, like Uniswap v2/v3 is doing to refund gas while collecting the data needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that if the debt limits are too high then you could receive a quote for an amount that cannot be filled because it would cause myAccount to become unhealthy.

My first impression is to just make sure that the debt limits are set appropriately. I don't really want to re-implement the health check logic here.

I think ideally this would be effectively a full simulation. OTOH, I'm actually not too worried about swaps failing, even if they received a valid quote. Sure it's not good and we should avoid as much as possible, but bots will certainly be doing a full simulation of their actions before they execute them anyway.

So yeah we should make sure common stuff like supply caps is handled, but misconfigured debt limit? I'm not sure TBH.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my prospective, the compute functions should return the exact simulation of a "swap" and how to (values) execute the swap; otherwise it does not make much sense. It's like if the preview* or max* functions from ERC4626 sometimes return the correct value and sometimes not.

Functions are APIs endpoints and should be reliable otherwise, how could you fully trust the result?

bots will certainly be doing a full simulation of their actions before they execute them anyway

So for what are these functions for? Who's the "end user"? I think that you should see these quote functions as "real" standards where you "must obey" the definitions (like for ERC4626) otherwise integrators won't use your service.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are not planning to fully support all the checks needed (caps, HF and so on) you should anyway document that the compute quote APIs are a "best effort" estimation and which are the cases where the value returned could be "wrong" and if used the swap execution could anyway revert.

}

function depositAssets(address vault, uint256 amount) internal {
IEVault(vault).deposit(amount, myAccount);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's effectively a minimum trade amount because if the shares = assets.toSharesDown() are 0 this reverts. but usually just a few wei

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh good point. That's a sort of an annoying edge case. A good argument for not failing on boundary conditions like this.

I guess we could call convertToShares() first and see if it's 0, but IMO it's probably not worth the extra gas overhead.

Comment on lines 168 to 176
if (debt > 0) {
IEVC(evc).call(
vault, myAccount, 0, abi.encodeCall(IBorrowing.repayWithShares, (type(uint256).max, myAccount))
);

if (debt <= amount) {
IEVC(evc).call(vault, myAccount, 0, abi.encodeCall(IRiskManager.disableController, ()));
}
}
Copy link

@MrToph MrToph Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. shares are rounded down in deposit.
  2. assets are rounded down in repayShares.

so the amount -deposit-> shares -repayShares-> amount' flow rounds down twice and you might not end up burning the same amount that you deposited.

Therefore, debt <= amount (depositAmount) does not mean that debt == 0 after deposit + repayShares. The disableController could still revert.

Could just check the debt balance directly:

if (myDebt(vault) == 0) {
    IEVC(evc).call(vault, myAccount, 0, abi.encodeCall(IRiskManager.disableController, ()));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Is it right to say that this would only occur when somebody does a swap of exactly the right size to zero out the debt (and no more)?

I was trying to avoid the call overhead here, but I think you're right that this is more correct and we should do it this way.

Copy link
Contributor Author

@hoytech hoytech Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds about 4300 gas to the median swap (+1.3%). I think it's fine, making this change, thx!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10

}

constructor(BaseParams memory params) EVCUtil(params.evc) {
require(params.fee < 1e18, BadFee());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a constant here instead of a "magic" number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so used to 1e18 that it hardly even looks magic to me anymore hehe.

I actually sort of like it since it self-documents the expected scale of the parameter. Besides, what would you even call this constant? I'm not a big fan of wad/ray terminology, and anything else feels less clear than just writing it out.

uint256 fee;
}

constructor(BaseParams memory params) EVCUtil(params.evc) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider emitting an event to track the deployment of new Maglev contracts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! Adding events are on the TODO list. We'll probably add an emit MaglevCreated or something here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we emit event here, how will the tracking works as the contract is not deployed yet? this is better handled in a factory style as in #7

Comment on lines 50 to 51
vault0 = params.vault0;
vault1 = params.vault1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some basic sanity checks on the user inputs like

  • params.vault0 != params.vault1
  • params.vault0.EVC() == params.evc
  • params.vault1.EVC() == params.evc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. @haythemsellami has already added a check for the first one (actually that the assets of the vaults differ, which is even more restrictive). EVC addresses is also a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12


require(quote <= (asset0IsInput ? reserve1 : reserve0), InsufficientReserves());
require(
quote <= IERC20(asset0IsInput ? asset1 : asset0).balanceOf(asset0IsInput ? vault1 : vault0),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should not rely on the token.balanceOf(vault)
Someone could have donated to the vault, but in reality the amount of cash available to be withdrawn/borrowed is lower than the balanceOf.

This should use vault.cash()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! This might even be slightly cheaper gas-wise, I'll measure and let you know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11

Comment on lines 198 to 202
require(quote <= (asset0IsInput ? reserve1 : reserve0), InsufficientReserves());
require(
quote <= IERC20(asset0IsInput ? asset1 : asset0).balanceOf(asset0IsInput ? vault1 : vault0),
InsufficientCash()
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This_computeQuote is called with

  • exactIn=true when amount is how much the user wants to sell of token0 and needs to know how much token1 it will receive
  • exactIn=false when amount is how much the user wants to receive (buy) of token1 and needs to know how much token0 needs to sell

This means that the semantics of amount and quote changes depending on the value of exactIn
When

  • exactIn == true, amount is the amount to sell, quote is the amount bought.
  • exactIn == false, amount is the amount bought, quote is the amount sold.

This means that both the require statement should use quote when exactIn == true BUT amount when exactIn == false

Otherwise, the _computeQuote(..., exactIn=false) could revert when the swap function would instead succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops you're right, thanks! We need some better testing for the edge cases when quoting amounts that exceed the limits/available cash. And for exactOut too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11

import {IUniswapV2Callee} from "./interfaces/IUniswapV2Callee.sol";
import {IMaglevBase} from "./interfaces/IMaglevBase.sol";

abstract contract MaglevBase is IMaglevBase, EVCUtil {
Copy link

@StErMi StErMi Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO]

  • consider renaming internal/private functions as _functionName
  • consider replacing the 1e18 magic number with a constant
  • consider emitting an ad-hoc event for the swap execution to monitor the swaps for security/analytics purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% re: the event.

We'll discuss the styling suggestions.

Comment on lines 59 to 60
reserve0 = offsetReserve(params.debtLimit0, vault0);
reserve1 = offsetReserve(params.debtLimit1, vault1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that all these parameters are immutable, I would consider reverting the deployment of the contract if, at deployment time, the README requirements are not fully respected.

In this case, you should check out if any balance has been deployed to the myAccount and if they make sense compared to the params.debtLimit0 and params.debtLimit1 inputs.

Let's assume this:

  1. T0: Alice deploys a Maglev with debtLimit0 = 0, debtLimit1 = 0 (which is a valid config)
  2. T1: Alice supplies funds to the connected account

After T1, the Maglev is configured with r0 = 0 and r1 = 0. While the account now has funds to be swapped without the need to take debt, it will always anyway revert for underflow because reserves are equal to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maglev doesn't handle changes to the NAV after deployment very well currently. I'm not sure if you saw @MrToph 's hackmd notes here: https://hackmd.io/@cmichel/mglv

He has a suggestion to dynamically adjust the reserves which we are still thinking through.

In this particular case, I think we'd be OK with requiring Alice to uninstall this Maglev instance and deploy a new one, although I agree this isn't optimal. However, no-debt instances are probably somewhat of a special case. I suspect in most cases if you changed your NAV (withdraw or deposit) you'd also want to change the debt limits to keep your maximum LTV constant.

}
}

function _computeQuote(address tokenIn, address tokenOut, uint256 amount, bool exactIn)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function reverts when amount or quote are equal to zero?

  • when exactIn=true and amount = 0 you are trying to swap zero for something which should be prevented
  • when exactIn=false and amount = 0 you are trying to get nothing for by selling something which would be shooting in your foot

The same logic should be applied to swap which is the "real" execution of the simulation done in _computeQuote

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I don't like failing on edge cases like this since you never know, maybe some contract is coded to swap N and sometimes that N happens to be 0.

I think it should be fine as long as exactIn=true returns 0 and exactOut returns >= 0 (might get padded up because of the rounding compensation in EulerSwap curve).

Either way, we should test these edge cases. 👍


// Swapper interface

function swap(uint256 amount0Out, uint256 amount1Out, address to, bytes calldata data)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The swap function allows the execution of swap(0, 0, receiver, "").
Should this function revert when amount0Out == 0 && amount1Out == 0 OR amount0In == 0 && amount1In == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous point: I think it's OK to allow a swap that does nothing. It's not really hurting anything and it might prevent unnecessary failures in integrating contracts in the future.

import {IUniswapV2Callee} from "./interfaces/IUniswapV2Callee.sol";
import {IMaglevBase} from "./interfaces/IMaglevBase.sol";

abstract contract MaglevBase is IMaglevBase, EVCUtil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it make sense to offer a permissionless reorderAccountCollaterals function that will execute a sequence of on-chain calculated evc.reorderCollaterals to reduce the iterations performed when a withdrawal/borrow operation triggers a validation check during a swap?

To prevent any abuses, the order should be calculated directly on chain by reorderAccountCollaterals.
Note that myAccunt could have more collaterals enabled compared to the one used as reserves here.

Having a well-ordered collateral could help reduce the gas cost of the swap operation because it would early return when the sanity checks are performed on the myAccount HF

Copy link
Contributor Author

@hoytech hoytech Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh interesting point! We'd have to measure, but I think the gas savings wouldn't be significant because if you have a debt in one of the vaults, it means you should not have a balance. 0 balances are special cased to not querying pricing anyway, so I think the number of calls to the oracle should be the same in both orderings.

If you have a debt, the vault will always be pricing both of the vaults regardless (one for collateral value and one for liability value.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the difference here is that your POV is about a "perfect" Maglev system, while mine is a Maglev that can be "messy".

To be more clean: you envision a Maglev where myAccount has only v1 and v2 enabled as collateral and none or only one has enabled an underlying token as collateral.

On my hand, I can't only take in consideration the happy path, but I need to also think about how it could end up because myAccount, the owner of myAccount or another operator could mess it up by providing more collaterals, and it could end up with a list of collaterals that is non-ordered and in the worst possible order.

The idea is that you want the checkLiquidity of LiquidityUtils to return as early as possible and enter the if (collateralValue > liabilityValue) return; branch.

Unfortunately, I need to consider those "messy" cases because EVK/EVC enables the user to end up in that situation. You could argue that the "official" Maglev won't end up in those situations because are "managed" by trusted entities, but I (as security researcher/dev) can't consider all the possible cases.

Comment on lines 173 to 175
IEVC(evc).call(
vault, myAccount, 0, abi.encodeCall(IBorrowing.repayWithShares, (type(uint256).max, myAccount))
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the withdrawAssets where the borrow is performed only if there's an amount left to be borrowed (not enough deposited liquidity) this logic will try to always extinguish the max debt, no matter what the amount deposited is compared to what have been supplied already (by previous tx on myAccount).

I think that is fine because you want anyway to extinguish the debt as soon as possible to pay less interest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this occurs if somebody transfers shares to myAccount for the vault it currently has a debt position in. Yeah this is a bit of an edge case. I actually did think about this although I forgot to document it.

I think it's fine because yes the shares will contribute to repaying the debt, thereby conserving NAV. The weird state where you have a liability and shares in the same vault will "solve itself" as swapping activity occurs into this vault, consuming the debt.

import {IUniswapV2Callee} from "./interfaces/IUniswapV2Callee.sol";
import {IMaglevBase} from "./interfaces/IMaglevBase.sol";

abstract contract MaglevBase is IMaglevBase, EVCUtil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brainstorming ideas to reduce gas for swapper and/or cost (interest) for LP
I'm not so sure how aggregators works, but I also assume that part of the ranking is given also by the gas cost of the operation, so lowering as much as possible would be good to be selected as a "source" of the swap in the aggregator's UI.

no-debt maglev

In this case, myAccount needs to provide all the liquidity to prevent any debt at any time to avoid to

  • perform enableController/disableController
  • perform borrow/repay
  • perform any sanity checks needed when a borrow/withdraw is performed

This not only reduce the gas cost but also increase the profitability of the maglev for the owner of myAccount that doesn't accrue any debt interest

The downsides are that

  1. you need to provide much more liquidity
  2. it cannot be "really" enforced on-chain. The owner of myAccount (or another operator) could break the Maglev assumption by borrowing (without the ability for the swap to repay/disable controller) but this could happen already with the current implementation if they (owner or operator) open a borrow position (enable controller) for an asset that is not asset0 or asset1

implement ad-hoc functions on Euler core to support on-shot operations

this would reduce the gas cost needed in the worst-case scenario where multiple external calls to EVC are needed to perform the swap

withdrawOrBorrow(amount) = withdraw(amount) + enable-controller-if-needed + borrow-if-needed
depositAndClose(amount) = deposit + repay(max)-when-debt + disableController-if-needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-debt maglev

I think the behaviour you describe is already supported by first depositing and then deploying an instance with both debt limits set to 0. Unless (as you mention) there are external changes to the balances then I don't think it will ever try to enable/disable controllers or borrow/repay, thereby consuming less gas. I could be wrong about this though -- I'll double check.

Note that this is supported by the gas report: There's a pretty big difference between best and worst case gas usage.

implement ad-hoc functions on Euler core to support on-shot operations

Yes, these would be useful composite functions, but the ship has already sailed on changing the EVK so we're stuck with the interface we have!

hoytech and others added 25 commits April 28, 2025 14:56
cosmetic fixes identified in audit
fix: add even stricter checks in fInverse() fuzz test
use safeTransfer when moving protocol fees, for non-compliant tokens
avoid use of assembly in getParams (suggested by @MiloTruck)
Constructor sanity check prevented creation of fully one-sided curves…
@hoytech
Copy link
Contributor Author

hoytech commented Apr 30, 2025

Out of date now. Closing to clean-up PRs.

@hoytech hoytech closed this Apr 30, 2025
hoytech added a commit that referenced this pull request Sep 11, 2025
hoytech added a commit that referenced this pull request Sep 11, 2025
hoytech added a commit that referenced this pull request Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants