Fix STATICCALL #4200
Fix STATICCALL #4200
Conversation
|
This also requires JSON tests update. I can't do it because I have issues with compiling LLL code. |
| m_runGas += toInt63(m_schedule->callValueTransferGas); | ||
|
|
||
| size_t sizesOffset = (m_OP == Instruction::DELEGATECALL || m_OP == Instruction::STATICCALL) ? 2 : 3; | ||
| size_t const sizesOffset = haveValueArg ? 3 : 2; |
There was a problem hiding this comment.
Did you forget a ! here or was there a bug before?
There was a problem hiding this comment.
I also swapped the 2 and 3.
libevm/VMCalls.cpp
Outdated
| } | ||
| else | ||
| { | ||
| callParams->apparentValue = callParams->valueTransfer = m_SP[2]; | ||
| inOutOffset = 1; | ||
| callParams->apparentValue = m_ext->value; |
There was a problem hiding this comment.
Do we want to forward the value in case of staticcall? I would say thiscase should only apply to delegatecall.
There was a problem hiding this comment.
You are probably right. In case of STATICALL the value should not be forwarded and be 0 all the time.
This might be another bug. @winsvega Can you look for some test cases for this?
There was a problem hiding this comment.
I have those on the list
actually callvalue has some strange value at this test
https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/stStaticCall/static_callToDelCallOpCodeCheckFiller.json#L43
There was a problem hiding this comment.
There was a problem hiding this comment.
All of them are quite complex. We need a simple test with a transaction having no-zero value, then staticcall and check of the callvalue inside this staticcall.
Similar one with a CALL instead of transaction.
There was a problem hiding this comment.
this test does exactly just that
just add more values to the value list
There was a problem hiding this comment.
I'm not sure what you mean by "this test". You referenced 2 tests before. If you mean https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/stStaticCall/static_callToStaticOpCodeCheckFiller.json#L43, this tests does other weird things like: checking for ORIGIN, multiple levels of different calls, etc. I'm not sure any of them check if the value from transaction is forwarded in STATICCALL.
Also, I don't know what you mean by "just add more values to the value list".
There was a problem hiding this comment.
I will add tests.
- call transaction into A with non-zero value; A performs STATICCALL to B; B stores the call value
- call transaction into Q; Q calls into A with non-zero value; A performs STATICCALL to B; B stores the call value
In both cases, the storage slot of B would initially contain some non-zero value, so that we can be sure that the value is overwritten by zero.
Codecov Report
@@ Coverage Diff @@
## develop #4200 +/- ##
===========================================
+ Coverage 67.35% 67.54% +0.19%
===========================================
Files 302 302
Lines 23257 23360 +103
===========================================
+ Hits 15664 15779 +115
+ Misses 7593 7581 -12
|
|
I added the two tests. |
|
Let me have a look at these
|
|
Ready to go? |
|
@winsvega sounds not happy about the tests I changed. |
|
I changed the tests and rendered them useless. I will apply correct fixes. |
Fixes #4199.