Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.6 receive fallback function instrumented #516

Open
elenadimitrova opened this issue Jun 10, 2020 · 8 comments
Open

0.6 receive fallback function instrumented #516

elenadimitrova opened this issue Jun 10, 2020 · 8 comments

Comments

@elenadimitrova
Copy link

transfer call to instrumented receive fallback function fails as it probably exceeds the transfer call gas stipend. Similar to #106

@cgewecke
Copy link
Member

cgewecke commented Jun 10, 2020

@elenadimitrova

Am looking at this right now... the test we have with a fallback that looks like this

    receive() external payable
    {
        if (msg.value > 0)
            emit Deposit(msg.sender, msg.value);
    }

...is passing. Client is ganache-core v2.10.1 (MuirGlacier).

It probably works because it's too simple and doesn't use much gas.

Can you show a receive example that's failing for you?

@elenadimitrova
Copy link
Author

The updated Proxy contract here is failing to receive value. I'll try to get a min working example for you.

@cgewecke
Copy link
Member

@elenadimitrova

Adding a third parameter to the event (bytes data) was enough to get it to fail on this side.

Is the fallback with the assembly working ok though?

@elenadimitrova
Copy link
Author

elenadimitrova commented Jun 10, 2020

Seems that way. There are ~70 occurrences of invokeWallet that does the low level call and they are working. That has sufficient gas passed to it though so it's different to doing a transfer. Why do you suspect it might be failing?

@cgewecke
Copy link
Member

Why do you suspect it might be failing?

There's some gas distortion caused by Solidity statements the tool injects to track the execution path. Hopefully it's possible to minimize those for receive and keep things under the stipend limit though.

Will try to fix this tomorrow and publish an experimental patch, and maybe you could check that it works for those contracts.

@cgewecke
Copy link
Member

@elenadimitrova 0.7.7 hopefully fixes. Just lmk if not and will revisit.

elenadimitrova added a commit to elenadimitrova/etherlime that referenced this issue Jun 11, 2020
@elenadimitrova
Copy link
Author

elenadimitrova commented Jun 11, 2020

Thanks @cgewecke but still having the same problem. receive was instrumented as follows:

receive() external payable {
coverage_0x583770bd(0x067553a7ab4566e3d100d5dd51a4d4d7c1dd78170cd2de2ede027b3713d158ca); /* line */ 
        if (msg.value > 0) {coverage_0x583770bd(0x6f53e18b49c038dc954f681591a954eba8d5f4cfb34caeeaa63ac41fda82672c); /* branch */ 

coverage_0x583770bd(0x76f304403c4c60d131d831b10dbe528a8037658735d6680c87b0f63310580e12); /* line */ 
            emit Received(msg.value, msg.sender, msg.data);
        }else { coverage_0x583770bd(0x62bc3173bb736c7ddf6032536275c27f047a20ac743535b1a36d2e4191f6f6d8); /* branch */ 
}
    }

@cgewecke
Copy link
Member

cgewecke commented Jun 11, 2020

@elenadimitrova Oh I'm sorry. That's strange.

We have very similar instrumentation in the test here and it's passing. These send & transfer examples:

...run this...

  event Deposit(address indexed _sender, uint _value, bytes data);

  receive() external payable
  {
    coverage_0xf2a277f5(0xb2baf8814b88a8e10df7811cfe4de699f4dfa82fee21030cef466b986ce61cb7); /* line */ 
    if (msg.value > 0){
      coverage_0xf2a277f5(0xa4b8e24690a0c94debffedca61a226696034c80f40f925c74914509c12313b8b); /* branch */ 
      coverage_0xf2a277f5(0xbbd5fa732f9422ad22c57d343d767a696fb98bd2e856cc657c377d4ebe8fe8ea); /* line */ 
      emit Deposit(msg.sender, msg.value, msg.data);
    } else { 
      coverage_0xf2a277f5(0xc454deeb9e6220ae9751a1a45b1b5f6cf82972ab7c93be8d76a188b9bc8a5c9e); /* branch */ 
    }
  }

Have also experimented with removing the indexed modifier in the event definition and it works.

What is the value of msg.data for you when that event fires normally? Here it's null, which I thought might be expected because the Solidity docs mention receive not accepting calldata.

elenadimitrova added a commit to argentlabs/argent-contracts that referenced this issue Jun 12, 2020
elenadimitrova added a commit to argentlabs/argent-contracts that referenced this issue Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants