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

fix for short address attack #188

Merged
merged 2 commits into from Apr 11, 2017

Conversation

Projects
None yet
8 participants
@miohtama

This comment has been minimized.

Copy link
Contributor

miohtama commented Apr 7, 2017

I read Golem blog post https://blog.golemproject.net/how-to-find-10m-by-just-reading-blockchain-6ae9d39fcd95 and I am trying to understand better what happened. Now there is a confirmation in the comments that this was a bug specific to the exchange implementation.

Though I see this as a risk in the future, I am not sure if this should be fixed like this. Rationale here is that the bug was not specific to a transfer function, but as far as I understand from the post, the exchange code would have encoded data any function call invalidly. Thus, one would have to make extra effort to protect all functions against invalid data payloads.

@maraoz

This comment has been minimized.

Copy link
Member

maraoz commented Apr 7, 2017

Personally I'm not sure if this should be fixed at the smart contract level. The attack seems like an implementation bug in how exchanges handle user inputs and craft transactions with them. If exchanges use standard tools and good security practices this shouldn't be needed.

That said, this seems like a low-cost protection that would add to the overall security of tokens. As such, it would be a step towards increasing in-depth security of OpenZeppelin contracts.

Let's have some community discussion before we merge this please. I'm not against but still not convinced.

@makoto

This comment has been minimized.

Copy link
Contributor

makoto commented Apr 7, 2017

Should have test coverage. Also CI test is failing...

@simondlr

This comment has been minimized.

Copy link

simondlr commented Apr 7, 2017

It's a useful addition, but I would agree: is the contract responsible for this security? It could be, and so your consideration arrives at cost of error vs cost of gas.

In the event of an error, you would still have to consider if all OpenZeppelin tokens over the course of its deployment won't add up to the cost of that error?

So you'll have to probably do some probability weights to determine if it's worth it. What's the likelihood of this attack being exploited and what's the cost associated with it (risk of event occurring * cost), then see if that adds to gas cost over a certain period of time.

@makoto

This comment has been minimized.

Copy link
Contributor

makoto commented Apr 7, 2017

Is the extra gas cost something we can quantify?

@simondlr

This comment has been minimized.

Copy link

simondlr commented Apr 7, 2017

Is the extra gas cost something we can quantify?

Yes. The cost of the modifier is additional gas cost vs not having the protection.

This gas cost for all Zeppelin tokens need to be added up and compared to the cost of losing value due to the protection not being there. Depends on how you weight the probability of these errors occurring, and what the expected losses will be.

@makoto

This comment has been minimized.

Copy link
Contributor

makoto commented Apr 7, 2017

@simondlr I mean the actual gas cost. Is it always x wei (if so how much is x?) or variable depending on other factors?

@simondlr

This comment has been minimized.

Copy link

simondlr commented Apr 7, 2017

Is it always x wei (if so how much is x?) or variable depending on other factors?

I assume so, yes. Essentially the cost of the extra code being deployed + the modifier being called for every transfer.

This line seems like a fixed cost.

assert(msg.data.length == size + 4);

@maurelian

This comment has been minimized.

Copy link
Contributor

maurelian commented Apr 7, 2017

Wouldn't the cost be "x gas", rather than "x wei"?

@makoto

This comment has been minimized.

Copy link
Contributor

makoto commented Apr 7, 2017

Looks like the discussion is already going on at Solidity level. ethereum/solidity#2109

Probably duplicate effort?

@izqui

This comment has been minimized.

Copy link
Contributor

izqui commented Apr 7, 2017

I think this 'more gas for more security' discussion is very interesting and goes beyond this specific issue.

After reading this ethereum/solidity#510, my personal opinion is that Zeppelin should have a 'StandardToken' and a 'UnsafeStandardToken'. The unsafe token would be cheaper to run for use cases where the gas difference is really a deal breaker, this implementation could lack also security measures such as using safe math.

I think the industry standard should be to add as much security as possible (even if some things look like an overkill and by having to pay the gas every time) to the contracts to prevent against hypothetic multi-million dollar looses. Once the contracts can pay for their own gas, this would be a decision up to the developer/creator to individually asses the risk and decide how much security she wants and wants to pay for.

@simondlr

This comment has been minimized.

Copy link

simondlr commented Apr 9, 2017

@izqui I like that approach.

The "standard" being the most secure contract, and then creating alternatives for developers that know what they are doing (substituting some security features for gas savings, for example).

@maraoz

This comment has been minimized.

Copy link
Member

maraoz commented Apr 11, 2017

@izqui @simondlr sounds good.
PR still failing for some reason. Will restart travis build and see what happens

@izqui

This comment has been minimized.

Copy link
Contributor

izqui commented Apr 11, 2017

Oh, it is the vested token test that is failing. The grantVestedTokens(...) function calls the transfer function under the hood and it is doing the assertion that the payload is 68 bytes, but the payload for grantVestedTokens is bigger.

Modifying the assertion to assert(msg.data.length >= size + 4) should fix this and the assertion would still work, as the problem is with smaller not with bigger payloads (where Solidity will throw for us, I think).

The name of the modifier should be changed to something like 'minimumPayloadSize'.

@jdetychey

Fix for bigger payloads
as suggested by izqui in case a function calls transfer under the hood
@jdetychey

This comment has been minimized.

Copy link
Contributor

jdetychey commented Apr 11, 2017

done thx

@izqui

This comment has been minimized.

Copy link
Contributor

izqui commented Apr 11, 2017

LGTM!

@maraoz

This comment has been minimized.

Copy link
Member

maraoz commented Apr 11, 2017

LGTM

@maraoz maraoz merged commit 22018fd into OpenZeppelin:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
* Fix for the ERC20 short address attack
*/
modifier onlyPayloadSize(uint size) {
assert(msg.data.length >= size + 4);

This comment has been minimized.

@chriseth

chriseth Apr 12, 2017

Contributor

As you are checking a condition on inputs, shouldn't this rather be a require()?

This comment has been minimized.

@izqui

izqui Apr 12, 2017

Contributor

The code is using Solidity v0.4.8, the assert function is implemented in SafeMath.sol, it is not the solidity one

Whenever Zeppelin migrates to v0.4.10 i agree that should be a require.

This comment has been minimized.

@chriseth

chriseth Apr 12, 2017

Contributor

Ah sorry and thanks for the clarification :-)

ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018

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