feat(precompiles): implement TIP-1099 keychain ABI changes - #7270
Conversation
|
b30c73d to
cda0149
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 6422/10631 lines (60.41%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6423/10833 lines (59.29%) |
|
cyclops audit fast |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change fork-gates direct AccountKeychain authorization selectors at T11 and replaces nested-ABI setAllowedCalls with RLP. One verified gas-pricing issue is annotated inline; one additional issue cannot be placed inline because its root-cause line is outside this PR's diff.
⚠️ [ISSUE] Non-direct calls bypass mandatory input-gas accounting
Severity: Low
File: crates/precompiles/src/lib.rs:254
tempo_precompile! rejects DELEGATECALL and CALLCODE before creating StorageCtx or invoking the implementation-level input charge, so the rejection consumes zero precompile gas regardless of input size. A T4 call with 33 input bytes and 11 gas returns DelegateCallNotAllowed instead of halting out of gas for the required 12-gas charge; at T11 it skips 60 gas.
Recommended Fix: Charge input_cost(spec, input.data.len()) before the non-direct-call return, halt out of gas when insufficient, and report the charged gas on DelegateCallNotAllowed. Add T4 and T11 regression tests.
Reviewer Callouts
- ⚡ Pricing consistency:
key_authorizationcharges 7,000 gas per target and selector plus 5,000 per recipient, while T11setAllowedCallscharges only about 57, 17.5, and 52 gas respectively for the same work. - ⚡ T11 Solidity coverage:
tips/verify/foundry.toml:23excludesAccountKeychainTestandAccountKeychainInvariantTest, leaving the new T11 surface without the existing Solidity invariant coverage. - ⚡ Storage-credit coverage: Tests in
crates/node/tests/it/storage_credits.rs:110andcrates/node/tests/it/tempo_transaction/local.rs:1535were pinned to T10 even though key authorization becomes the only T11 provisioning route and disables TIP-1060 storage credits during keychain writes. - ⚡ SDK compatibility:
crates/alloy/src/provider/keychain.rs:371always emits the T11-only selector, so the helper reverts on pre-T11 networks and has no legacy counterpart.
| call: setAllowedCallsCall, | ||
| ) -> Result<()> { | ||
| if !self.storage.spec().is_t3() { | ||
| self.storage.deduct_gas(rlp_input_cost(call.scopes.len()))?; |
There was a problem hiding this comment.
🚨 [SECURITY] RLP pricing omits per-element validation work
The T11 path charges 80 gas per 32-byte input word, then runs unmetered HashSet inserts in validate_call_scopes and validate_selector_rules. A minimal selector rule occupies only 7 RLP bytes, costing about 17.5 gas per forced insertion. End-to-end measurements put this path at roughly 0.27–0.50 Ggas/s instead of TIP-1099's 1 Ggas/s target, and it is about 27% cheaper per selector validation than the ABI path it replaces. A duplicate final selector forces the complete validation pass and reverts before any storage write.
Recommended Fix:
Charge a per-scope, per-selector, and per-recipient surcharge before validation, or reprice the per-word surcharge from end-to-end decode-and-validation benchmarks. Add a large duplicate-tail regression test that asserts the intended gas charge.
96c6c9a to
a0599d6
Compare
a0599d6 to
49b6975
Compare
Co-authored-by: Derek Cofausper <256792747+decofe@users.noreply.github.com>
49b6975 to
52b08d0
Compare
| key_id: Address, | ||
| scopes: Vec<CallScope>, | ||
| ) -> Result<()> { | ||
| if !self.storage.spec().is_t3() { |
There was a problem hiding this comment.
it is a bit odd this is called reached by rlp path when we can assume t3 is active since it will always be t11+
| let mut encoded_scopes = call.scopes.as_ref(); | ||
| let scopes = Vec::<RlpCallScope>::decode(&mut encoded_scopes) | ||
| .map_err(|_| AccountKeychainError::invalid_call_scope())?; | ||
| if !encoded_scopes.is_empty() { |
There was a problem hiding this comment.
does the rlp decoder not require it to be consumed?
There was a problem hiding this comment.
guess not but can switch to decode_exact IIUC
Depends on tempoxyz/tempo-std#139.
Fork-gates direct AccountKeychain authorization selectors at T11 and replaces nested-ABI setAllowedCalls with the TIP-1011 RLP schema. RLP input is charged another 50 gas per 32-byte word before decoding, on top of TIP-1100 input pricing.