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

Remove short address attack checks from tokens #261

Closed
maraoz opened this Issue Jun 16, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@maraoz
Member

maraoz commented Jun 16, 2017

If transfer and transferFrom are used by a subcontract's function with less arguments, the checks will fail.

@maraoz

This comment has been minimized.

Show comment
Hide comment
@maraoz

maraoz Jun 16, 2017

Member

@izqui thoughts? This happened to me recently and had to use a horrible workaround

Member

maraoz commented Jun 16, 2017

@izqui thoughts? This happened to me recently and had to use a horrible workaround

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Jun 16, 2017

Contributor

+1 to removing it. An honorable effort, indeed, but it was done at the wrong layer and in response to a single isolated case of very poor validation in the upper layers.

Contributor

federicobond commented Jun 16, 2017

+1 to removing it. An honorable effort, indeed, but it was done at the wrong layer and in response to a single isolated case of very poor validation in the upper layers.

@izqui

This comment has been minimized.

Show comment
Hide comment
@izqui

izqui Jun 16, 2017

Member

I agree. I haven't even deployed this in our own token.

Member

izqui commented Jun 16, 2017

I agree. I haven't even deployed this in our own token.

@DavidKnott

This comment has been minimized.

Show comment
Hide comment
@DavidKnott

DavidKnott Jun 20, 2017

Contributor

@federicobond Just trying to learn, what layer do you think short address attack protection should be built into?

Contributor

DavidKnott commented Jun 20, 2017

@federicobond Just trying to learn, what layer do you think short address attack protection should be built into?

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Jun 20, 2017

Contributor

It should be handled by the libraries that read the contract ABI and let you create transaction payloads out of it (web3.js, truffle-artifactor, etc.)

Contributor

federicobond commented Jun 20, 2017

It should be handled by the libraries that read the contract ABI and let you create transaction payloads out of it (web3.js, truffle-artifactor, etc.)

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Jun 20, 2017

Contributor

Think of this analogy: dynamically-loaded libraries should not (and probably cannot) verify that you have pushed the right arguments into the stack before calling them. They just expose their ABI as header files and (unless you are writing assembly by hand) any high-level language compiler worth its salt will take care of the rest. What the library code SHOULD do is try to make sense of the arguments assuming they were pushed the right way, or bail out if not.

Contributor

federicobond commented Jun 20, 2017

Think of this analogy: dynamically-loaded libraries should not (and probably cannot) verify that you have pushed the right arguments into the stack before calling them. They just expose their ABI as header files and (unless you are writing assembly by hand) any high-level language compiler worth its salt will take care of the rest. What the library code SHOULD do is try to make sense of the arguments assuming they were pushed the right way, or bail out if not.

@frangio

This comment has been minimized.

Show comment
Hide comment
@frangio

frangio Jun 26, 2017

Member

I'd like to add too that the modifier is incredibly error prone because the size argument has to be calculated and kept up-to-date by the programmer.

Member

frangio commented Jun 26, 2017

I'd like to add too that the modifier is incredibly error prone because the size argument has to be calculated and kept up-to-date by the programmer.

@frangio

This comment has been minimized.

Show comment
Hide comment
Member

frangio commented Jun 26, 2017

frangio added a commit to frangio/openzeppelin-solidity that referenced this issue Jun 26, 2017

@maraoz maraoz closed this in #277 Jun 26, 2017

LefterisJP added a commit to LefterisJP/raiden-token that referenced this issue Oct 4, 2017

Remove isValidPayload()
As also seen here:
OpenZeppelin/openzeppelin-solidity#261 this check
is kind of an overkill and should not be made at the smart contract
layer as it can have undesired consequences.

LefterisJP added a commit to LefterisJP/raiden-token-1 that referenced this issue Oct 4, 2017

Remove isValidPayload
Removing isValidPayload as also seen here:
OpenZeppelin/openzeppelin-solidity#261 it's an
overkill check that should be done in a different layer and not at the
smart contracts.

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

@rpsnoopy

This comment has been minimized.

Show comment
Hide comment
@rpsnoopy

rpsnoopy Jun 18, 2018

How can you prevent malicious user to send the short address to your contract if the error is trapped at the user interface level only? The user interface is out of control for any public function: you can write your own and call whatever. And what about "it should be trapper at web3 level"? Is it trapped or not?

rpsnoopy commented Jun 18, 2018

How can you prevent malicious user to send the short address to your contract if the error is trapped at the user interface level only? The user interface is out of control for any public function: you can write your own and call whatever. And what about "it should be trapper at web3 level"? Is it trapped or not?

@frangio

This comment has been minimized.

Show comment
Hide comment
@frangio

frangio Jun 18, 2018

Member

Web3.js correctly handles this. I don't remember right now if it pads an address argument to its full length, or it just rejects a short one. This was never a problem with web3.js. The attack was done on a website which used its own handwritten transaction encoder.

Member

frangio commented Jun 18, 2018

Web3.js correctly handles this. I don't remember right now if it pads an address argument to its full length, or it just rejects a short one. This was never a problem with web3.js. The attack was done on a website which used its own handwritten transaction encoder.

@rpsnoopy

This comment has been minimized.

Show comment
Hide comment
@rpsnoopy

rpsnoopy Jun 18, 2018

Given this, definitively I will use short address mitigation in my contracts, simply using >= instead of == in the msg.data.length check. “Should” be managed at another level is not enough safe to me.

Given this, definitively I will use short address mitigation in my contracts, simply using >= instead of == in the msg.data.length check. “Should” be managed at another level is not enough safe to me.

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