feat(universal-token): add eip-2612 permit/nonces/domain separator#377
feat(universal-token): add eip-2612 permit/nonces/domain separator#377hedwig0x wants to merge 2 commits intofluentlabs-xyz:develfrom
Conversation
📝 WalkthroughWalkthroughAdds ERC‑2612 permit support: domain separator and permit digest computation, nonce storage and queries, signature-based allowance setting (via ecrecover), SDK commands/constants for permit/nonces, storage key handling, dependency bump, and end-to-end tests for valid and invalid signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Signer
participant Client as Caller
participant Contract as UniversalToken
participant Crypto as ecrecover
participant Storage as ContractStorage
User->>Client: Produce permit parameters & sign digest (v,r,s)
Client->>Contract: call permit(owner, spender, value, deadline, v, r, s)
Contract->>Contract: compute EIP‑712 domain separator
Contract->>Contract: compute permit struct hash
Contract->>Contract: build final digest ("\x19\x01" || separator || permitHash)
Contract->>Crypto: ecrecover(digest, v, r, s)
Crypto-->>Contract: recovered signer
Contract->>Contract: verify recovered == owner
alt valid signature
Contract->>Storage: set allowance[owner][spender] = value
Contract->>Storage: increment nonce[owner]
Contract-->>Client: success
else invalid signature or expired
Contract-->>Client: revert (ERR_UST_INVALID_SIGNATURE / ERR_UST_EXPIRED_DEADLINE)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/src/universal_token.rs (1)
53-61: Minor code duplication withcontracts/universal-token/lib.rs.The
abi_word_addrandabi_word_u256helpers duplicate the implementations incontracts/universal-token/lib.rs(lines 257-265). While acceptable for test isolation, consider extracting to a shared utility in the SDK if these patterns are needed elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/src/universal_token.rs` around lines 53 - 61, The helpers abi_word_addr and abi_word_u256 in e2e/src/universal_token.rs duplicate logic found in contracts/universal-token/lib.rs; extract these functions into a shared utility module in the SDK (e.g., sdk::abi_utils or similar), move the implementations of abi_word_addr(Address) and abi_word_u256(U256) into that module, then replace the local definitions with imports/uses of the shared functions in both places so the tests and contract code reuse a single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/universal-token/lib.rs`:
- Around line 281-303: Add a SECP256K1N_HALF constant using the provided
U256::from_limbs([...]) little-endian limbs and enforce signature
non-malleability in ecrecover_address by rejecting any signature where s >
SECP256K1N_HALF; implement the check near the start of ecrecover_address (before
proceeding to rec_id logic) and return None when the test fails, using U256
comparison semantics to compare the incoming s to SECP256K1N_HALF so malleable
signatures are rejected.
---
Nitpick comments:
In `@e2e/src/universal_token.rs`:
- Around line 53-61: The helpers abi_word_addr and abi_word_u256 in
e2e/src/universal_token.rs duplicate logic found in
contracts/universal-token/lib.rs; extract these functions into a shared utility
module in the SDK (e.g., sdk::abi_utils or similar), move the implementations of
abi_word_addr(Address) and abi_word_u256(U256) into that module, then replace
the local definitions with imports/uses of the shared functions in both places
so the tests and contract code reuse a single implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d3682b0-afa9-4fae-8b03-f966dda9e5f8
⛔ Files ignored due to path filters (1)
contracts/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
contracts/universal-token/Cargo.tomlcontracts/universal-token/lib.rscrates/sdk/src/universal_token/command.rscrates/sdk/src/universal_token/consts.rscrates/sdk/src/universal_token/storage.rse2e/src/universal_token.rs
| fn ecrecover_address(digest: B256, v: u8, r: U256, s: U256) -> Option<Address> { | ||
| let rec_id = match v { | ||
| 27 | 28 => v - 27, | ||
| 0 | 1 => v, | ||
| _ => return None, | ||
| }; | ||
|
|
||
| let mut sig_bytes = [0u8; 64]; | ||
| sig_bytes[0..32].copy_from_slice(&abi_word_u256(r)); | ||
| sig_bytes[32..64].copy_from_slice(&abi_word_u256(s)); | ||
| let sig = <&B512>::try_from(&sig_bytes[..]).ok()?; | ||
|
|
||
| let recovered = ecrecover(sig, rec_id, &digest).ok()?; | ||
| let mut recovered_addr = [0u8; 20]; | ||
| recovered_addr.copy_from_slice(&recovered[12..32]); | ||
| let recovered_addr = Address::from_slice(&recovered_addr); | ||
|
|
||
| if recovered_addr == Address::ZERO { | ||
| return None; | ||
| } | ||
|
|
||
| Some(recovered_addr) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What is the secp256k1 curve order N value and how should EIP-2 signature malleability be checked?
💡 Result:
The secp256k1 curve order N is 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141, or in decimal: 115792089237316195423570985008687907852837564279074904382605163141518161494337. EIP-2 addresses ECDSA signature malleability in Ethereum's Homestead hard fork by making signatures with s-value greater than secp256k1n/2 invalid. To check for EIP-2 signature malleability compliance (i.e., enforce low-s), verify that the s value satisfies s <= N/2, where N/2 is 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0. If s > N/2, the signature is malleable (equivalent to N - s would be the low-s version) and should be rejected. This is implemented in Ethereum's ecrecover precompile post-Homestead and recommended in Solidity contracts using manual checks or libraries like OpenZeppelin's ECDSA.
Citations:
- 1: https://crypto.stackexchange.com/questions/53597/how-did-someone-discover-n-order-of-g-for-secp256k1
- 2: https://www.secg.org/sec2-v2.pdf
- 3: https://en.bitcoin.it/wiki/Secp256k1
- 4: https://learnmeabitcoin.com/technical/cryptography/elliptic-curve/
- 5: https://std.neuromancer.sk/secg/secp256k1/
- 6: https://eips.ethereum.org/EIPS/eip-2
- 7: https://ethereum.stackexchange.com/questions/83174/is-it-best-practice-to-check-signature-malleability-in-ecrecover
- 8: https://bips.dev/62/
- 9: https://bips.dev/146
🏁 Script executed:
# First, check if the file exists and examine the ecrecover_address function
find . -name "lib.rs" -path "*/universal-token/*" | head -5Repository: fluentlabs-xyz/fluentbase
Length of output: 103
🏁 Script executed:
# Verify the U256 limb conversion for SECP256K1N_HALF
python3 << 'PYTHON_EOF'
# Correct N/2 in big-endian hex
N_HALF_HEX = "0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0"
# Convert to int
n_half = int(N_HALF_HEX, 16)
# Extract limbs (little-endian, each limb is 64-bit)
limb0 = n_half & ((1 << 64) - 1)
limb1 = (n_half >> 64) & ((1 << 64) - 1)
limb2 = (n_half >> 128) & ((1 << 64) - 1)
limb3 = (n_half >> 192) & ((1 << 64) - 1)
print("Correct N/2 limbs (little-endian):")
print(f"Limb 0: 0x{limb0:016X}")
print(f"Limb 1: 0x{limb1:016X}")
print(f"Limb 2: 0x{limb2:016X}")
print(f"Limb 3: 0x{limb3:016X}")
print("\nProposed constant in review:")
proposed = [0xFFFFFFFFFFFFFFFF, 0xFFFFFFFFFFFFFFFE, 0xBAAEDCE6AF48A03B, 0x7FFFFFFFFFFFFFFF]
for i, limb in enumerate(proposed):
print(f"Limb {i}: 0x{limb:016X}")
print("\nDo they match?")
print(f"Match: {[limb0, limb1, limb2, limb3] == proposed}")
PYTHON_EOFRepository: fluentlabs-xyz/fluentbase
Length of output: 378
🏁 Script executed:
# Read the ecrecover_address function from the file
sed -n '281,303p' ./contracts/universal-token/lib.rsRepository: fluentlabs-xyz/fluentbase
Length of output: 778
🏁 Script executed:
# Check if there are any other permit/signature-related implementations in the codebase
rg -A 10 -B 5 "permit\|erc20_permit" ./contracts/universal-token/lib.rs | head -100Repository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Check if malleability check is done elsewhere in the codebase
rg "SECP256K1\|s >\|s_value\|malleab" ./contracts/Repository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Read more context around the ecrecover_address function
sed -n '250,330p' ./contracts/universal-token/lib.rsRepository: fluentlabs-xyz/fluentbase
Length of output: 2490
🏁 Script executed:
# Check if there's a permit handler that calls ecrecover_address
sed -n '324,400p' ./contracts/universal-token/lib.rs | head -80Repository: fluentlabs-xyz/fluentbase
Length of output: 2397
Consider adding signature malleability check and correct the constant value.
The ecrecover_address function doesn't verify that the s value is in the lower half of the curve order (EIP-2). While this doesn't enable replay attacks (the nonce is consumed), it's a best practice to reject malleable signatures to match standard EIP-2612 implementations like OpenZeppelin's.
The proposed constant must be corrected before implementation. The correct SECP256K1N_HALF U256 limbs in little-endian format are:
U256::from_limbs([
0xDFE92F46681B20A0,
0x5D576E7357A4501D,
0xFFFFFFFFFFFFFFFF,
0x7FFFFFFFFFFFFFFF,
])
Add the check before the rec_id match to reject any signature with s > SECP256K1N_HALF.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/universal-token/lib.rs` around lines 281 - 303, Add a
SECP256K1N_HALF constant using the provided U256::from_limbs([...])
little-endian limbs and enforce signature non-malleability in ecrecover_address
by rejecting any signature where s > SECP256K1N_HALF; implement the check near
the start of ecrecover_address (before proceeding to rec_id logic) and return
None when the test fails, using U256 comparison semantics to compare the
incoming s to SECP256K1N_HALF so malleable signatures are rejected.
dmitry123
changed the title
1 participant
Summary
permit(owner,spender,value,deadline,v,r,s)nonces(owner)andDOMAIN_SEPARATOR()handlers + selector dispatchImplementation notes
name, version1,chainId,verifyingContract).revm_precompile::secp256k1::ecrecover.ERR_UST_EXPIRED_DEADLINEERR_UST_INVALID_SIGNATUREValidation
cargo test --manifest-path contracts/universal-token/Cargo.toml --lib -- --nocapturecargo test -p fluentbase-e2e universal_token_permit -- --nocaptureSummary by CodeRabbit
New Features
Tests