Intrinsic arbitrage from price discrepancy

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/6b4867491350f8327d0ac4f496f263642cf3c1be/contracts/AfEth.sol#L148-L169

Vulnerability details

Impact

The up to 2 % price discrepancy from Chainlink creates an intrinsic arbitrage. Especially, it makes withdrawals worth more than deposits in the sense that one can immediately withdraw more than just deposited.

Proof of Concept

When depositing ETH into AfEth, the ETH is split according to ratio and sold for safEth and vAfEth. The received share of afEth is then determined by the value in ETH of the resulting amounts of safEth and vAfEth. Note that there are two prices involved here: the true price at which ETH is traded for safEth and vAfEth (in sellCvx() and buyCvx()), and the estimated value in ETH that safEth and vAfEth is considered to have (ISafEth.approxPrice() and VotiumStrategy.price()). These are not necessarily the same.

If the ratio by which the deposited ETH is split is not the same as the ratio of the true values of the underlying assets, this implies that a deposit implicitly makes a trade between safEth and vAfEth according to the estimated price which may thus differ from the true price obtained when withdrawing. This presents an arbitrage opportunity.
Note that if all prices were the same it would not matter if safEth is "traded" for vAfEth within a deposit as the trade then makes no change in the total value deposited.

The conditions for this issue is thus that VotiumStrategy.price() is different from the price obtained by sellCvx() and buyCvx(), and that the deposit ratio is not the same as the withdrawal ratio.

VotiumStrategy.price() in particular is based on a Chainlink oracle with a 2 % deviation threshold, which means that the true price is allowed to deviate up to 2 %, within 24 hours, from the reported price. ISafEth.approxPrice() may perhaps be similarly inaccurate (I have not looked into this).

The ratio can skew in this way for two reasons. One is when the ratio is different from the ratio of the underlying balances, such as when ratio is changed. Deposits are made according to ratio but withdrawals are made proportionally from the extant balances. In this case the implicit trade between safEth and vAfEth can happen in either direction, either beneficial or detrimental to the depositor (if there is a price discrepancy).
The other is caused by the price discrepancy itself when depositing. In this case it is always beneficial to the depositor (and detrimental to the holders).

Example 1a - reconverging ratio, vAfEth is actually more expensive
Suppose the contract holds 100 safEth and 0 vAfEth, but that the ratio has now been changed to 0. Further suppose that the contract thinks all prices are 1, but that 100 ETH actually trades for 102 vAfEth.
Then depositing 100 ETH will convert it to 102 vAfEth, which will be valued as 102 ETH. This mints 102 afEth.
The balances are now 100 safEth and 102 vAfEth and the total supply is 202 afEth.
Withdrawing 102 afEth converts 102/202 of the underlying, i.e. 50.495 safEth and 51.505 vAfEth, into 50.495 + 51.505/1.02 = 100.99 ETH.

Example 1b - reconverging ratio, vAfEth is actually cheaper
Suppose the contract holds 100 safEth and 0 vAfEth, but that the ratio has now been changed to 0. Further suppose that the contract thinks all prices are 1, but that 100 ETH actually trades for 98 vAfEth.
Then depositing 100 ETH will convert it to 98 vAfEth, which will be valued as 98 ETH. This mints 98 afEth.
The balances are now 100 safEth and 98 vAfEth and the total supply is 198 afEth.
Withdrawing 98 afEth converts 98/198 of the underlying, i.e. 49.495 safEth and 48.505 vAfEth, into 49.495 + 48.505/0.98 = 98.99 ETH.

Example 2a - stable ratio, vAfEth is actually more expensive
Suppose the contract holds 50 safEth and 50 vAfEth and that the ratio is 0.5. Further suppose that the contract thinks all prices are 1 but that 50 ETH actually trades for 51 vAfEth.
Then depositing 100 ETH will convert 50 ETH to 50 safEth and 50 ETH to 51 vAfEth, which will be valued as 101 ETH. This mints 101 afEth.
The balances are now 100 safEth and 101 vAfEth and the total supply is 201 afEth.
Withdrawing 101 afEth converts 101/201 of the underlying, i.e. 50.249 safEth and 50.751 vAfEth, into 50.249 + 50.751/1.02 = 100.005 ETH.

Example 2b - stable ratio, vAfEth is actually cheaper
Suppose the contract holds 50 safEth and 50 vAfEth and that the ratio is 0.5. Further suppose that the contract thinks all prices are 1 but that 50 ETH actually trades for 49 vAfEth.
Then depositing 100 ETH will convert 50 ETH to 50 safEth and 50 ETH to 49 vAfEth, which will be valued as 99 ETH. This mints 99 afEth.
The balances are now 100 safEth and 99 vAfEth and the total supply is 199 afEth.
Withdrawing 99 afEth converts 99/199 of the underlying, i.e. 49.749 safEth and 49.251 vAfEth, into 49.749 + 49.251/0.98 = 100.005 ETH.

Thus one can make a profit by depositing and immediately withdrawing. Immediate withdrawals are possible at the moment locks expire (and before they have been relocked), but it may be enough to just immediately request a withdrawal if the true price is the same (or better) when eventually withdrawn.

The price discrepancy will appear whenever there are price fluctuations of up to 2 % within 24 hours, which seems quite likely.

Regarding the case where the underlying is reconverging after a change of ratio it is worth noting that convergence is quite slow. Several times the entire balances must be traded before the new ratio is approached.

Recommended Mitigation Steps

I would have to think more about this.

Assessed type

Math

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[d3e4]

Recommended Mitigation Steps

We want that a deposit not diminish the value of previous deposits. That is, withdrawing $w$ shares should return at least as much if withdrawn after a deposit which mints $m$ shares as if withdrawn before.
Note that letting a share represent each underlying in equal proportions is the only way to guarantee fairness and fungibility, so we must leave the withdrawal calculation as it is.

Let $d_s$ and $d_v$ be the ether amounts deposited in SafEth and VotiumStrategy, respectively. Let $B_s$ and $B_v$ be the respective balances in AfEth and $T$ the total supply of afEth. Let $P_s(x)$ be the amount of safEth obtained by selling $x$ ether for safEth, and $P_s^{-1}(y)$ the amount of ether obtained by selling safEth for ether (note the abuse of notation and that $P_s^{-1}(P_s(x)) \leq x$ because of fees, spread, slippage etc.), and similarly $P_v$ and $P_v^{-1}$ for vAfEth.

Withdrawing $w$ now should return at most what it would return after a deposit of $d_s + d_v$, i.e.

$$P_s^{-1}(\frac{w}{T}B_s) + P_v^{-1}(\frac{w}{T}B_v) \leq P_s^{-1}(\frac{w}{T+m}(B_s + P_s(d_s))) + P_v^{-1}(\frac{w}{T+m}(B_v + P_v(d_v)))$$

For small deposits and withdrawals the price functions are approximately linear and we can write e.g. $P_s(d_s)$ as $P_s d_s$, i.e. $P_s$ is just a price point, and we get

$$\frac{1}{T}(P_s^{-1}B_s + P_v^{-1}B_v) \leq \frac{1}{T+m}(P_s^{-1}(B_s + P_s d_s) + P_v^{-1}(B_v + P_v d_v))$$$

Solving for $m$ we get

$$m \leq \frac{P_s^{-1}P_s d_s + P_v^{-1}P_v d_v}{P_s^{-1}B_s + P_v^{-1}B_v}T$$

The difference from the current implementation is that instead of the true prices $P_s^{-1}$ and $P_v^{-1}$, the oracle prices, which we will denote $\hat{P}^{-1}_s$ and $\hat{P}^{-1}_v$, are used instead, i.e.

$$m = \frac{\hat{P}^{-1}_s P_s d_s + \hat{P}^{-1}_v P_v d_v} {\hat{P}^{-1}_s B_s + \hat{P}^{-1}_v B_v}T$$

Since we know the true prices up to within certain bounds, we can minimise $m$, as a function of $P_s^{-1}$ and $P_v^{-1}$, within these bounds.
The gradient of $m(P_s^{-1}, P_v^{-1})$ is

$$\frac{P_s d_s B_v - P_v d_v B_s} {(P^{-1}_s B_s + P^{-1}_v B_v)^2} (P^{-1}_v, -P^{-1}_s)$$

so if $P_s d_s B_v - P_v d_v B_s > 0$ we pick the lower right corner (maximal $P_s^{-1}$, minimal $P_v^{-1}$), and if $P_s d_s B_v - P_v d_v B_s < 0$ we pick the upper left corner (minimal $P_s^{-1}$, maximal $P_v^{-1}$).
In the case of equality we can use any (non-zero) prices.

We minimise within the bounds, but we of course want the bounds to be as tight as possible, so that this minimum is as high as possible.

The oracle prices provide us with good bounds, namely $P_s^{-1} \in [0.98 \cdot \hat{P}^{-1}_s, 1.02 \cdot \hat{P}^{-1}_s]$ and $P_v^{-1} \in [0.98 \cdot \hat{P}^{-1}_v, 1.02 \cdot \hat{P}^{-1}_v]$.

There are a few ways to further improve the bounds. During the deposit we learn $P_s$ and $P_v$, from which we can infer $P_s^{-1} \leq 1/P_s$ and $P_v^{-1} \leq 1/P_v$ (equality in the case of no exchange losses). If we know that there is some minimum percentage lost (e.g. exchange fees, slippage, spread) we can refine these to $P_s^{-1} \leq k_s/P_s$ and $P_v^{-1} \leq k_v/P_v$, where $k_s, k_v < 1$ is some factor adjusting for these losses (e.g. $k_s = 0.99$ if there is at least (another) 1 % lost when selling for ether).

If the trading losses are significant it may be necessary to take these into account even for the bounds from the oracle prices, such that both upper and lower bounds are slightly reduced.

[romeroadrian]

@0xleastwood I'm really sorry to do this at this stage, but did you have the chance to go through these scenarios? Seems there are lot of suppositions, some of which the same warden is confusingly invalidating in other discussions.

I think examples 1a and 1b have invalid assumptions ("the contract holds 100 safEth and 0 vAfEth, but that the ratio has now been changed to 0"). In 2a and 2b, what is "the contract thinks all prices are.."? How is the contract thinking prices?

It seems this has nothing to do with the stated impact. The chainlink response is used to price vAfEth, but the core here is a discrepancy of the ratio with the underlying assets (which again confusingly the author is trying to invalidate in other issues). Furthermore the deposit/withdraw cycle can't be executed without exposure to the underlying assets due to the locking mechanism.

[0xleastwood]

@0xleastwood I'm really sorry to do this at this stage, but did you have the chance to go through these scenarios? Seems there are lot of suppositions, some of which the same warden is confusingly invalidating in other discussions.

I think examples 1a and 1b have invalid assumptions ("the contract holds 100 safEth and 0 vAfEth, but that the ratio has now been changed to 0"). In 2a and 2b, what is "the contract thinks all prices are.."? How is the contract thinking prices?

It seems this has nothing to do with the stated impact. The chainlink response is used to price vAfEth, but the core here is a discrepancy of the ratio with the underlying assets (which again confusingly the author is trying to invalidate in other issues). Furthermore the deposit/withdraw cycle can't be executed without exposure to the underlying assets due to the locking mechanism.

I'll look into these, I do believe it is possible to deposit and withdraw atomically as long as there is an unlocked amount of tokens in the votium strategy contract. That would be the extent at which a withdrawal could be made "instantly".

[0xleastwood]

I do agree that example 1a and 1b are somewhat infeasible because the protocol team has already stated that such a ratio would not exist in the first place. However, there is validity in the other examples.

[elmutt]

Thanks for the excellent analysis @d3e4, this one really got us thinking.

After extensive analysis we came to the following conclusions:

1. It can only happen if the true ratio of safEth to vStrategy in the contract is not equal to the target ratio. We were unable to reproduce this issue if targetRatio ~= trueRatio (see test https://github.com/asymmetryfinance/afeth/pull/180/files#diff-761b4f70c9a884d0e0afc44238412de69eaa5b3037a2490c6d5ba5d34d061218R207).

2. We were able reproduce the issue by mocking the chainlink oracle to significantly changing the cvxPerEth price and changing the ratio to be very different from the true ratio (see test https://github.com/asymmetryfinance/afeth/pull/180/files#diff-761b4f70c9a884d0e0afc44238412de69eaa5b3037a2490c6d5ba5d34d061218R267)

3. It appears that a 2% chainlink difference can cause small differences if the ratio is far enough apart. We wrote 2 tests demonstrating this:
   https://github.com/asymmetryfinance/afeth/pull/180/files#diff-761b4f70c9a884d0e0afc44238412de69eaa5b3037a2490c6d5ba5d34d061218R331
   https://github.com/asymmetryfinance/afeth/pull/180/files#diff-761b4f70c9a884d0e0afc44238412de69eaa5b3037a2490c6d5ba5d34d061218R398

These are a little bit concerning We believe this is an acceptable risk because:

• in another pull request we set a minimum withdraw time of 1 epoch so its impossible to instantly withdraw even if there are unlockable funds in the contract.
• its risky for them to try this because they are exposed to cvx price changes most of the time unlock time will be much greater than 1 week because there wont be enough unlockable cvx to withdraw right away.
• we will also have a bot monitoring the ratio and chainlink price and pause the contract if there are any major issues

4. We also analyzed the amountToMint calculation in afEth.deposit() as follows:

amountToMint = totalValue / priceBeforeDeposit

or

((safEthMinted * safEthPrice) + (vStratMinted * vStratPrice)) / priceBeforeDeposit

or

((safEthMinted * safEthPrice) + (vStratMinted * vStratPrice)) / ((totalVStratValue + totalSafEthValue) / totalAfEthSupply)

or

((safEthMinted * safEthPrice) + (vStratMinted * vStratPrice)) / (((totalVstratBalance * vStratPrice) + (totalSafEthBalance * safEthPrice)) / totalAfEthSupply)

If we assume safEthPrice is constant (in terms of eth it goes up very slowly) so we can pretty much ignore it (replace it with a constant)

or

((safEthValueMinted) + (vStratMinted * vStratPrice)) / (((totalVstratBalance * vStratPrice) + (totalSafEthValue)) / totalAfEthSupply)

Assigning values generated when ratios were roughly equal to everything except vStratPrice and plotting it looks like:

We think this graph helps show that if ratios are equal it is hard to mess things up with a bad chainlink price. We understand ratios will often not be equal but we wanted to see the base case of ratios being equal

Based on this analysis we think a 2% chainlink variance is an acceptable risk, even when the ratios are far apart. However its possible we missed something and if anyone can demonstrate in a unit test that its a bigger problem we would love to see it!

[elmutt]

Also wanted to point out the ratios can easily change if cvx price changes, so it it a real concern. We initially didnt think about it right and assumed ratios would be more stable