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

contract can lose the funds! #1

Open
amiller opened this Issue Aug 27, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@amiller

amiller commented Aug 27, 2015

You should check in the cash function that the send instruction completes. send can silently fail due to a call-stack error. If that happens, the execution carries on, isCashed is set, and the pot becomes lost forever.

An attacker can cause this to happen with a carefully-formed transaction that preloads the callstack to depth 1023. Here's a demonstration of this against lotto.sol in pyethereum:
https://gist.github.com/amiller/665cc46970f2c0684d2a#file-test_etherpot-py

This is a known hazard in writing smart contracts. We reported on it as part of an independent Ethereum security audit:
https://github.com/LeastAuthority/ethereum-analyses/blob/master/GasEcon.md#callstack-depth-limit-errors

The best work-around here is defensive programming. The send failure is not exactly silent, the caller can (and should) check the return value of send as a boolean. This is also demonstrated (in Serpent, for now) in the online course documents from University of Maryland.
http://mc2-umd.github.io/ethereumlab/

@aakilfernandes

This comment has been minimized.

Contributor

aakilfernandes commented Aug 27, 2015

This is incredibly useful, looking into it now.

@aakilfernandes

This comment has been minimized.

Contributor

aakilfernandes commented Aug 27, 2015

Has the Ethereum team ever commented on that issue? I can't seem to find any official word on how to prevent these kinds of attacks.

@CJentzsch

This comment has been minimized.

CJentzsch commented Aug 27, 2015

Its a feature, not a bug :-)
The call stack depth limit of 1024 is important to assure consensus. Because every machine would fail on a different stack depth without it. The protocol clearly defines what happens at a call at stack depth 1024 (YP page 29 section "CALL"). It will return 0. Its the task of the contract programmer to check whether a call was successful or not. A call can also fail due to different reasons (e.g. out of gas), therefore it is always important to check the return value.

@aakilfernandes

This comment has been minimized.

Contributor

aakilfernandes commented Aug 27, 2015

When you say It will return 0 does that mean I can do a simple check like so in solidity:

if(person.send(...)===0) return;
@amiller

This comment has been minimized.

amiller commented Aug 27, 2015

In Solidity it's a bool. The simplest fix is if (!winner.send(subpot)) return;

@aakilfernandes

This comment has been minimized.

Contributor

aakilfernandes commented Aug 27, 2015

Gracias!

@aakilfernandes

This comment has been minimized.

Contributor

aakilfernandes commented Aug 27, 2015

should probably wait to fix before actually closing =p

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