eth-educators / ethstaker-deposit-cli

Secure key generation for deposits
https://eth-educators.github.io/ethstaker-deposit-cli/
Creative Commons Zero v1.0 Universal
3 stars 5 forks source link

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

Open remyroy opened 3 months ago

remyroy commented 3 months ago

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 commented 3 months ago

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

To be confirmed.

remyroy commented 3 months ago

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 commented 1 month ago

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 commented 4 weeks ago

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 commented 3 weeks ago

The document part should be in #140