Openzeppelin-contracts: Remove short address attack checks from tokens

[maraoz]

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

[maraoz]

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

[federicobond]

+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]

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

[DavidKnott]

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

[federicobond]

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]

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]

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]

For reference, here's a demo of the error @maraoz mentions:
https://ethereum.github.io/browser-solidity/#gist=f5c444b9e087d03438aa990cb91b9e3a

[rpsnoopy]

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]

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]

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.

[elopio]

For the record, this was fixed in Solidity 0.5.0: https://github.com/ethereum/solidity/pull/4224