Fix issue with detection of RIP7212 precompile #5620
Conversation
🦋 Changeset detectedLatest commit: f640dca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
Co-authored-by: Arr00 <[email protected]>
Co-authored-by: Arr00 <[email protected]>
Co-authored-by: Arr00 <[email protected]>
| let success := staticcall(gas(), 0x100, ptr, 0xa0, 0x00, 0x20) | ||
| isValid := and(success, and(eq(returndatasize(), 0x20), eq(mload(0), 1))) | ||
| } |
There was a problem hiding this comment.
Just a little paranoid on the gas estimation part.
// The `invalid` upon `staticcall` failure is solely for gas estimation.
// When given sufficient gas, the precompile will not revert.
if iszero(staticcall(gas(), 0x100, m, 0xa0, 0x00, 0x00)) { invalid() }
// The precompile will only return `uint256(1)` if and only if the signature is valid.
// As per latest spec, verified against the op-geth implementation:
// See: https://github.com/ethereum-optimism/op-geth/blob/02dfe8692a3c606dbabd83c1ced2037aab9753d7/core/vm/contracts.go#L1342
// Otherwise, it will always return no data.
isValid := iszero(iszero(returndatasize()))More conservative (in case the spec and implementations are somehow edited again):
// The `invalid` upon `staticcall` failure is solely for gas estimation.
// When given sufficient gas, the precompile will not revert.
mstore(0x00, 0) // Zeroize the slot for the returndata.
if iszero(staticcall(gas(), 0x100, m, 0xa0, 0x00, 0x20)) { invalid() }
// The precompile will only return `uint256(1)` if and only if the signature is valid.
// As per latest spec, verified against the op-geth implementation:
// See: https://github.com/ethereum-optimism/op-geth/blob/02dfe8692a3c606dbabd83c1ced2037aab9753d7/core/vm/contracts.go#L1342
// Otherwise, it will always return no data.
isValid := mload(0x00)There was a problem hiding this comment.
I'd like to avoid the invalid() here. I think its better if the function returns false instead of reverting if we are in a strange situation where the precompile acts eraticly. But again, if the precompile revert ... maybe something very fishy is going on and its ok to fail.
There was a problem hiding this comment.
also, aren't ifs quite expensive compared to basic and operations?
There was a problem hiding this comment.
should we worry about the precompile returning something that is neither 0 nor 1 ?
There was a problem hiding this comment.
I think we don't need to worry about it.
There was a problem hiding this comment.
I like @Vectorized's idea since the library is designed to work with RIP-7212 so it should be safe to assume that all implementations would be compliant (i.e. returning empty bytes on failed verification). We can add a note so that developers can just override the function and manually check the returndata, but I'm afraid the consequences of missing such note would be pretty bad.
Co-authored-by: Ernesto García <[email protected]>
There was a problem hiding this comment.
Approving because I agree with the functionality. However, I'm leaving another nit. Feel free to merge if you don't agree @Amxx.
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Arr00 <[email protected]> Co-authored-by: Ernesto García <[email protected]>
Fixes #5619
PR Checklist
npx changeset add)