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

msg.sender.send() not working for contract -> contract-based wallets transactions #135

Closed
emosto opened this Issue Jan 30, 2016 · 9 comments

Comments

Projects
None yet
7 participants
@emosto

emosto commented Jan 30, 2016

When a contract tries to send ether using msg.sender.send() to a contract-based wallet it fails (without error). However, msg.sender.send() works if the sender is the etherbase account. A workaround I found was to use tx.origin.send() but this then sends the amount to the etherbase account and not to the calling wallet.

Shouldn't msg.sender.send() do its job no matter where the address points (contract or account)?

@emosto

This comment has been minimized.

emosto commented Jan 31, 2016

I'm sorry, this issue should have been attached to the dapp-meteor-wallet project I guess..

@frozeman

This comment has been minimized.

Member

frozeman commented Feb 2, 2016

Thats more of a question for solidly: @chriseth

EDIT: i think this problem occurs, as the wallet is using tx.origin to determine the "from" when a tx is received, we need to do this as we deploy stub wallets.

So basically the money should arrive, its just wrongly credited in the log

@simondlr

This comment has been minimized.

simondlr commented Feb 2, 2016

Could this be related to the inherent limitation in contract to contract send()? It only has the gas stipend it can use. No other gas is forwarded. http://solidity.readthedocs.org/en/latest/frequently-asked-questions.html#what-is-the-deal-with-function-inside-solidity-contracts-how-can-a-function-not-have-a-name. It seems the contract wallet would have enough gas to do that fallback function (check if msg.value and do a log). But it might be just too much (not sure if the if statement could be screwing this up). Did anyone check/test on this?

(A work-around is to use address.call.value(_value)(_data) instead as this forward gas).

@frozeman

This comment has been minimized.

Member

frozeman commented Feb 2, 2016

I think the problem comes from us, using stub contracts when generating wallets. These use call code to "access" the code of the "original" wallet.
This wallet code then uses tx.origin instead of msg.sender, as msg.sender would refer to the contract calling the code and not the (before) last contract or transaction sender.

@emosto

This comment has been minimized.

emosto commented Feb 2, 2016

Thanks for picking it up, guys.There is another reported issue related to this behavior, I have the feeling - #76

@chriseth

This comment has been minimized.

Contributor

chriseth commented Feb 3, 2016

I think @simondlr's analysis is correct. The stub correctly receives the transfer and forwards it using CALLCODE to the actual code but also has to send the ether again. This could cost more ether than the stipend allows.
If this is correct, then this will be fixed in homestead, where we have better contract stubs.

@kieranelby

This comment has been minimized.

kieranelby commented Feb 20, 2016

For what it's worth, I got bitten by this surprising interaction between address.send() and wallet contracts here: http://www.kingoftheether.com/postmortem.html

@frozeman frozeman added the Type: Bug label Feb 22, 2016

@chfast

This comment has been minimized.

chfast commented Oct 29, 2016

I'm not sure where the Wallet code is, but it looks to be fixed in https://github.com/ethereum/meteor-dapp-wallet/blob/develop/Wallet.sol#L334-L338.

@lock

This comment has been minimized.

lock bot commented Mar 30, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Mar 30, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.