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

Consider implementing a check on msg.data.length #8

Open
GriffGreen opened this issue May 1, 2017 · 3 comments
Open

Consider implementing a check on msg.data.length #8

GriffGreen opened this issue May 1, 2017 · 3 comments

Comments

@GriffGreen
Copy link
Member

GriffGreen commented May 1, 2017

There was an attack vector that an address that ends in 0 or 00 or 000 etc could use their address and omit the final 0's and then mess with the data length.

I love how the Token Card guys dealt with it.

https://github.com/MonolithDAO/token/blob/master/src/Token.sol#L23

    modifier onlyPayloadSize(uint numwords) {
        assert(msg.data.length == numwords * 32 + 4);
        _;

I might suggest we consider changing the verbiage, but the intent and application is great, in every function that a user has to input msg.data that includes and address, this modifier is used... this means transfer(), transferFrom(), approve() and also decreaseApproval() and increaseApproval() if we use it. (see my other issue ;-))

My suggested verbiage:

    modifier onlyValidDataLengths(uint numOfParams) {
        assert(msg.data.length == numOfParams * 32 + 4);
        _;
@jbaylina
Copy link
Contributor

PR #15 implements it.

But I'm not convinced on this, because this functions might be called internally from a derived contract and what's checking are not the parameters but the data field.

@koeppelmann
Copy link

please be careful doing this. There is some low level solidity padding happening that might cause the payload to be bigger than expected. So while a check against a too small payload should be fine a check for "equal" is dangerous.
It might lead to a situation that those tokens could get stuck in e.g. a multisig wallet.

Read more here: https://blog.coinfabrik.com/smart-contract-short-address-attack-mitigation-failure/
or here:
http://vessenes.com/notice-about-fun-token-support-for-multisignature-wallets-2/

@acedev0816
Copy link

assert(msg.data.length == numwords * 32 + 4);
why +4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants