saturating operations for `v` in `Signature`

[tcoratger]

Saturating operations for v in Signature to avoid potential overflow.

[tcoratger]

hmm technically chainid can be u64, so this sounds reasonable, although probably a bad choice to pick a chainid where this is possible so maybe not worth it, but also doesn't really hurt

wdty @Rjected

I noticed this during random testing but I think it can apply in production too. If the chain ID is too large (greater than the max of u64/2) then the result of the operation will be an overflow and will panic. So for security, even if most of the time the chain IDs are much smaller, it may be better to put this.

[Rjected]

hmm technically chainid can be u64, so this sounds reasonable, although probably a bad choice to pick a chainid where this is possible so maybe not worth it, but also doesn't really hurt

wdty @Rjected

I noticed this during random testing but I think it can apply in production too. If the chain ID is too large (greater than the max of u64/2) then the result of the operation will be an overflow and will panic. So for security, even if most of the time the chain IDs are much smaller, it may be better to put this.

This has little relevance to security if it is still inconsistent with how other clients handle a very large chain ID. For example the saturating ops are no better than wrapping if geth uses a u256 for chainid

[tcoratger]

hmm technically chainid can be u64, so this sounds reasonable, although probably a bad choice to pick a chainid where this is possible so maybe not worth it, but also doesn't really hurt wdty @Rjected

I noticed this during random testing but I think it can apply in production too. If the chain ID is too large (greater than the max of u64/2) then the result of the operation will be an overflow and will panic. So for security, even if most of the time the chain IDs are much smaller, it may be better to put this.

This has little relevance to security if it is still inconsistent with how other clients handle a very large chain ID. For example the saturating ops are no better than wrapping if geth uses a u256 for chainid

Then I would say that we should reimplement this https://github.com/alloy-rs/alloy/blob/55a278c9db4e7e9b18b2bc2a290bd3de675d09f7/crates/rpc-types/src/eth/transaction/mod.rs#L35 instead of deriving. Indeed, if you use this for testing/fuzzing, you can have a chain_id that is > u64::max/2 and the test will panic if you try to do some operation with v() no?

[Rjected]

hmm technically chainid can be u64, so this sounds reasonable, although probably a bad choice to pick a chainid where this is possible so maybe not worth it, but also doesn't really hurt wdty @Rjected

I noticed this during random testing but I think it can apply in production too. If the chain ID is too large (greater than the max of u64/2) then the result of the operation will be an overflow and will panic. So for security, even if most of the time the chain IDs are much smaller, it may be better to put this.

This has little relevance to security if it is still inconsistent with how other clients handle a very large chain ID. For example the saturating ops are no better than wrapping if geth uses a u256 for chainid

Then I would say that we should reimplement this https://github.com/alloy-rs/alloy/blob/55a278c9db4e7e9b18b2bc2a290bd3de675d09f7/crates/rpc-types/src/eth/transaction/mod.rs#L35 instead of deriving. Indeed, if you use this for testing/fuzzing, you can have a chain_id that is > u64::max/2 and the test will panic if you try to do some operation with v() no?

In this case the fuzz test would be incorrect. This is why fuzz tests often need to constrain inputs, again this is a user defined value and it should be fine to recommend not choosing values above some number. I have another reason why saturating operations are incorrect - it makes reversing the operation impossible. This would also happen if we did use U256 for chainid, which I don't think would be worth it

[Rjected]

Would note that having a chainid this close to the boundary causes problems for both saturating and wrapping, for example if the resulting v were 27/28 in wrapping. In saturating ops the v is incorrect, in wrapping ops the v can be reversed but is ambiguous compared to old legacy txs

[mattsse]

closing as wont fix