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

Improper use of assert() in BasicToken and other places #435

Closed
3sGgpQ8H opened this issue Sep 11, 2017 · 6 comments

Comments

@3sGgpQ8H
Copy link

commented Sep 11, 2017

Method transfer of BasicToken smart contract uses method sub defined in SafeMath library to implicitly check that sender's balance is enough to transfer:

balances[msg.sender] = balances[msg.sender].sub(_value);

Method sub internally uses assert() function like this (a and b and method parameters):

assert(b <= a);

So, when sender's balance is insufficient, assert will fail and execution will be aborted. According to Solidity docs:

assert(bool condition): abort execution and revert state changes if condition is false (use for internal error)

but insufficient sender's balance is not internal error, so assert() should not be used here.

Note, that require() does not fit as well because:

require(bool condition): abort execution and revert state changes if condition is false (use for malformed input or error in external component)

and insufficient sender's balance is neither malformed input, not error in external component. So the best fit would be to use revert() here.

@frangio

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

Thanks for reporting @mikhail-vladimirov! I agree with your assessment. Do you want to submit a PR changing to revert?

@3sGgpQ8H

This comment has been minimized.

Copy link
Author

commented Sep 11, 2017

@frangio I personally strongly prefer return false over revert(), my motivation is here: ethereum/EIPs#610 (comment). So I prefer somebody else to submit this PR.

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

@mikhail-vladimirov sorry to disagree, require is meant to be used for validations, while assert should be used in order to prevent conditions which should never be possible, in this case, we are preventing an overflow.

I highly recommend this article, a detailed explanation about this subject.

@b-mueller

This comment has been minimized.

Copy link

commented Sep 25, 2018

Sorry to re-vitalize this old issue but I agree @3sGgpQ8H and @frangio. assert is meant assert invariants, i.e. conditions that are impossible to occur. If there's a way to violate and invariant it always means that there's a bug in the code.

The asserts in SafeMath are implicitly performing input validation and IMO should be replaced by require.

@nventuro

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

Hey @b-mueller, this was discussed recently in #1120, where we decided to switch over to require (#1187).

@b-mueller

This comment has been minimized.

Copy link

commented Sep 25, 2018

Ahh, I didn't notice! Awesome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.