Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Commit

Permalink
fix(callop): fix LastCalleeId, and other fixes (#1594)
Browse files Browse the repository at this point in the history
### Description

1. for callop, when there is no bytecode (empty account or non-existed
account) or pre check fails, the return data should be flushed to empty,
and the LastCalleeId (used in RETURNDATACOPY opcode) should be set to
correct callee id instead of 0.
2. precompile can fail. (eg: out of gas)
3. "precompile account exists" is only the current status of eth
mainnet. It is not part of evm spec. Following spec, precompile account
can be non-existed.
4. when ErrDepth, there is no transfer
5. (not 100% sure) I also changed the 'is_static' check for only call
opcode. It was a fix i made long ago during fixing each fail case of
testool. I guess maybe we can construct this test case to check this
behavior: delegatecall inside callcode(with non 0 value) inside
staticcall. It is a valid tx, but the 'value' is not zero?

### Issue Link

[_link issue here_]

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update
  • Loading branch information
lispc authored Sep 12, 2023
1 parent 320d31d commit 7449c4f
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 22 deletions.
8 changes: 4 additions & 4 deletions bus-mapping/src/evm/opcodes/callop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
let is_precompile = code_address
.map(|ref addr| is_precompiled(addr))
.unwrap_or(false);
// CALLCODE does not need to do real transfer
// Transfer value only when all these conditions met:
// - The opcode is CALL
// - The precheck passed
Expand All @@ -185,7 +186,7 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
&mut exec_step,
call.caller_address,
call.address,
callee_exists || is_precompile,
callee_exists,
false,
call.value,
)?;
Expand Down Expand Up @@ -229,7 +230,6 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
match (is_precheck_ok, is_precompile, is_empty_code_hash) {
// 1. Call to precompiled.
(true, true, _) => {
assert!(call.is_success, "call to precompile should not fail");
let caller_ctx = state.caller_ctx_mut()?;
let code_address = code_address.unwrap();
let (result, contract_gas_cost) = execute_precompiled(
Expand Down Expand Up @@ -282,7 +282,7 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
// 2. Call to account with empty code.
(true, _, true) => {
for (field, value) in [
(CallContextField::LastCalleeId, 0.into()),
(CallContextField::LastCalleeId, call.call_id.into()),
(CallContextField::LastCalleeReturnDataOffset, 0.into()),
(CallContextField::LastCalleeReturnDataLength, 0.into()),
] {
Expand Down Expand Up @@ -356,7 +356,7 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
// 4. insufficient balance or error depth cases.
(false, _, _) => {
for (field, value) in [
(CallContextField::LastCalleeId, 0.into()),
(CallContextField::LastCalleeId, call.call_id.into()),
(CallContextField::LastCalleeReturnDataOffset, 0.into()),
(CallContextField::LastCalleeReturnDataLength, 0.into()),
] {
Expand Down
43 changes: 25 additions & 18 deletions zkevm-circuits/src/evm_circuit/execution/callop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<F: Field> ExecutionGadget<F> for CallOpGadget<F> {
);
});

cb.condition(call_gadget.has_value.clone(), |cb| {
cb.condition(is_call.expr() * call_gadget.has_value.clone(), |cb| {
cb.require_zero(
"CALL with value must not be in static call stack",
is_static.expr(),
Expand Down Expand Up @@ -180,20 +180,17 @@ impl<F: Field> ExecutionGadget<F> for CallOpGadget<F> {
// skip the transfer (this is necessary for non-existing accounts, which
// will not be crated when value is 0 and so the callee balance lookup
// would be invalid).
let transfer = cb.condition(
is_call.expr() * not::expr(is_insufficient_balance.expr()),
|cb| {
TransferGadget::construct(
cb,
caller_address.to_word(),
callee_address.to_word(),
not::expr(call_gadget.callee_not_exists.expr()),
0.expr(),
call_gadget.value.clone(),
&mut callee_reversion_info,
)
},
);
let transfer = cb.condition(is_call.expr() * is_precheck_ok.expr(), |cb| {
TransferGadget::construct(
cb,
caller_address.to_word(),
callee_address.to_word(),
not::expr(call_gadget.callee_not_exists.expr()),
0.expr(),
call_gadget.value.clone(),
&mut callee_reversion_info,
)
});

// For CALLCODE opcode, verify caller balance is greater than or equal to stack
// `value` in successful case. that is `is_insufficient_balance` is false.
Expand Down Expand Up @@ -231,12 +228,18 @@ impl<F: Field> ExecutionGadget<F> for CallOpGadget<F> {
let stack_pointer_delta =
select::expr(is_call.expr() + is_callcode.expr(), 6.expr(), 5.expr());
let memory_expansion = call_gadget.memory_expansion.clone();

// handle calls to accounts with no code.
cb.condition(
and::expr(&[no_callee_code.expr(), is_precheck_ok.expr()]),
|cb| {
// Save caller's call state
for field_tag in [
cb.call_context_lookup_write(
None,
CallContextFieldTag::LastCalleeId,
Word::from_lo_unchecked(callee_call_id.expr()),
);
for field_tag in [
CallContextFieldTag::LastCalleeReturnDataOffset,
CallContextFieldTag::LastCalleeReturnDataLength,
] {
Expand Down Expand Up @@ -276,11 +279,15 @@ impl<F: Field> ExecutionGadget<F> for CallOpGadget<F> {
},
);

// handle is_insufficient_balance step transition
// handle is_insufficient_balance or !is_depth_ok step transition
cb.condition(not::expr(is_precheck_ok.expr()), |cb| {
// Save caller's call state
for field_tag in [
cb.call_context_lookup_write(
None,
CallContextFieldTag::LastCalleeId,
Word::from_lo_unchecked(callee_call_id.expr()),
);
for field_tag in [
CallContextFieldTag::LastCalleeReturnDataOffset,
CallContextFieldTag::LastCalleeReturnDataLength,
] {
Expand Down

0 comments on commit 7449c4f

Please sign in to comment.