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

Document and enforce the limit on the amount field in the deposit file [TOB-EF-DEP-004] #72

Open
remyroy opened this issue Jun 18, 2024 · 5 comments

Comments

@remyroy
Copy link
Member

remyroy commented Jun 18, 2024

TOB-EF-DEP-004 in the original security assessment notes that the Integer range for JSON numeric types is limited to [-(2**53)+1, (2**53)-1].

We should probably document and enforce some sane limits on our uses of integer with the amount field.

@remyroy
Copy link
Member Author

remyroy commented Jun 18, 2024

Our range is likely:
1000000000
32000000000 (0x00, 0x01)
2048000000000 (0x02)

To be confirmed.

@remyroy
Copy link
Member Author

remyroy commented Jun 18, 2024

More good insight from yorick:

1:47 PM]yorickdowne: For the documentation, you can cite the deposit contract. Having this in gwei with a floor of 1000000000 and a ceiling of 2048000000000 is not a random convention: Floor and gwei denomination is directly from the deposit contract deployed at 0x00000000219ab540356cbb839cbe05303d7705fa on mainnet, other networks accordingly
// Check deposit amount
require(msg.value >= 1 ether, "DepositContract: deposit value too low");
require(msg.value % 1 gwei == 0, "DepositContract: deposit value not multiple of gwei");
uint deposit_amount = msg.value / 1 gwei;
require(deposit_amount <= type(uint64).max, "DepositContract: deposit value too high");

The ceiling of 2048000000000 is established by EIP-7251. Should the code in future allow specifying the deposit size (it probably should), it will cap at 2048 ETH.

@yorickdowne
Copy link

Which is to say, we will not run into JSON limits. We are already enforcing a limit when we get user input for the deposit amount (new flow), capped at 2048000000000, or set it to 32000000000 (old flow).

This is far, far away from 2**53. This is not a security concern, we just need to push back on the finding and point out that we are, indeed, checking the range of this parameter to be between 1000000000 and 2048000000000

@valefar-on-discord
Copy link
Collaborator

The current cap for the partial-deposit amount parameter is 2**64 - 1 as is the case in the smart contract.
I forgot about this discussion and will update the cap.

@remyroy
Copy link
Member Author

remyroy commented Sep 10, 2024

The document part should be in #140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants