Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Account's stETH balance getting lower on 1 or 2 wei due to rounding down integer math #442

Open
TheDZhon opened this issue Jul 7, 2022 · 5 comments
Labels
next-upgrade Things to pickup for the next protocol upgrade

Comments

@TheDZhon
Copy link
Contributor

TheDZhon commented Jul 7, 2022

We had been got already familiar with the 1 wei corner case before a protocol-wide regression test coverage was introduced. Since the integration tests adoption moment we have been discovering the possibility to face the 2 wei corner case. It happens due to the fact that stETH balance calculation depends on two integer divisions (each one has 1 wei "loss" at most).

It should be addressed somehow in the future to prevent dealing with such peculiarities, especially for the upcoming Withdrawals support (Capella hardfork). Thus, I mark the issue with the "next-upgade" label here to catch up during a development stage.

@TheDZhon TheDZhon added the next-upgrade Things to pickup for the next protocol upgrade label Jul 7, 2022
@TheDZhon TheDZhon changed the title stETH balance getting lower on 1 or 2 wei due to rounded down integers account's stETH balance getting lower on 1 or 2 wei due to rounded down integers Jul 7, 2022
@TheDZhon TheDZhon changed the title account's stETH balance getting lower on 1 or 2 wei due to rounded down integers account's stETH balance getting lower on 1 or 2 wei due to rounding down integer math Jul 7, 2022
@TheDZhon TheDZhon changed the title account's stETH balance getting lower on 1 or 2 wei due to rounding down integer math Account's stETH balance getting lower on 1 or 2 wei due to rounding down integer math Jul 7, 2022
@TheDZhon
Copy link
Contributor Author

TheDZhon commented Jul 12, 2022

Possible explanation (made on the basis of the original statement by @skozin):

Let's have the following designations:

  • x is the number of tokens passed to the transfer function;
  • x' is the actual balance increase for the destination addr;
  • rate is the total pooled ether / total stETH shares (changes as time goes by, according to the actual APR)
  • floor is the usual integer rounding down

Given that:

shares_to_transfer = floor(x / rate); // stETH transfers implemented as shares transfers internally
x' = floor(shares_to_transfer * rate); // balance is calculated by multiplying shares number on rate

and

x' ∈ {[shares_to_transfer * rate - 1, shares_to_transfer * rate] ⋂ ℕ}
shares_to_transfer ∈ {[x / rate - 1; x / rate] ⋂ ℕ}

we may conclude:

x' ∈ {[(x / rate - 1) * rate - 1, (x / rate) * rate] ⋂ ℕ}

or, simplifying finally:

x' ∈ {[x - rate - 1, x] ⋂ ℕ}

thus, transferring x is guaranteed to actually transfer (i.e. balance increase x') an integer number between (x - rate - 1) and x.

Since rate currently is ≈1.08, max error is limited to 2 wei.

While the current APR is 4%, after the Merge it might increase to as high as 8%,
that gives rate ≈ 1.47 after 5 years (max error 2 wei)
after 10 years rate ≈ 2.17 (max error 3 wei)
after 20 years — rate ≈ 4.7 (max error 5 wei).

@sakulstra
Copy link

This issue is currently breaking collateralSwap and repayWithCollateral features on the aave protocol which have now been disabled on the decentralized interface for this very reason.
Also it's hard to reason about flashloans as it's hard to repay exact amounts when there's a +-2 wei margin.

Would love to see this resolved 👍

@Ekzer
Copy link

Ekzer commented Nov 6, 2023

Hi, we just got the case :
image
https://etherscan.io/token/0xae7ab96520de3a18e5e111b5eaab095312d7fe84?a=0xf570746d04915f4df7816d18438da11da244d60c
in event logs, we can see the transfer event of 395.97 stETH. However, from smart-contract perspective, there is this 1 wei delta unfortunately
image
image

@mymphe mymphe unpinned this issue Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-upgrade Things to pickup for the next protocol upgrade
Projects
None yet
Development

No branches or pull requests

6 participants
@TheDZhon @sakulstra @Ekzer and others