Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Trace precompiled contracts when the transfer value is not zero #8486

Merged
merged 5 commits into from
May 9, 2018
Merged

Trace precompiled contracts when the transfer value is not zero #8486

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
08eddce
Trace precompiled contracts when the transfer value is not zero
sorpaas Apr 25, 2018
6084acb
Add tests for precompiled CALL tracing
sorpaas Apr 27, 2018
e3d7ae3
Use byzantium test machine for the new test
sorpaas Apr 27, 2018
4e63a9a
Add notes in comments on why we don't trace all precompileds
sorpaas May 8, 2018
585d307
Use is_transferred instead of transferred
sorpaas May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ethcore/src/executive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,12 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
self.state.discard_checkpoint();
output.write(0, &builtin_out_buffer);

// trace only top level calls to builtins to avoid DDoS attacks
if self.depth == 0 {
// trace only top level calls and calls with balance transfer to builtins to avoid DDoS attacks
let transferred = match params.value {
ActionValue::Transfer(value) => value,
ActionValue::Apparent(_) => U256::zero(),
};
if self.depth == 0 || transferred != U256::zero() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it open up an attack vector then? The comment clearly states that only top level calls should be traced, currently the workaround is to just send 1 wei with them, which seems pretty cheap.

Also I would simplify it to:

let is_value_transfer = match params.value {
  Transfer(value) => value.is_zero(),
  Apparent(_) => false,
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomusdrw I'm a little bit confused here: isn't CALL/CALLCODE always pays the opcode fee regardless whether it's precompiled contract or not? If gases are properly paid then it's probably not an attack vector any more?

I noticed the original line was written by Gavin Wood before the EIP150 and Homestead hard fork. Given that we fixed most of the CALL gas attack vectors in those two hard forks, and the reasoning above, I actually think we may want to consider remove the depth == 0 check altogether.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the math also checks out. Consider 0x00..03 and 0x00..ff as two nonexisting accounts, using byzantium_test config, we have:

  • 6000600060006000600160ff61fffff1: CALL to 0x00..ff consumes 32421 gases for toplevel (without finalization).
  • 60006000600060006001600361fffff1: CALL to 0x00..03 consumes 33021 gases for toplevel (without finalization). The additional 600 gases are the base gas for RIPEMD160.

So I think it's safe to remove depth == 0 check altogether.

let mut trace_output = tracer.prepare_trace_output();
if let Some(out) = trace_output.as_mut() {
*out = output.to_owned();
Expand Down