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

address.call(bytesVar) results in corrupted data in the call #2884

Closed
lastperson opened this Issue Sep 12, 2017 · 12 comments

Comments

Projects
4 participants
@lastperson
Copy link
Contributor

lastperson commented Sep 12, 2017

If the bytesVar is a bytes param from a function call, then passing it to address.call will result in the input being zero padded to 32*n bytes.

Which version of Solidity you are using: 0.4.16 (reproducible on 0.3.5 too)
What was the source code (if applicable):

pragma solidity 0.4.16;
contract Caller {
    Caller2 receiver;
    function Caller(Caller2 _rec) {
        receiver = _rec;
    }
    function forward() {
        receiver.forward(msg.data);
    }
}
contract Caller2 {
    event Debug(uint, bytes);
    address receiver;
    function Caller2(address _rec) {
        receiver = _rec;
    }
    function forward(bytes _data) {
        Debug(_data.length, _data);
        receiver.call(_data);
    }
}
contract Receiever {
    event DebugReceive(uint, bytes);
    function () {
        DebugReceive(msg.data.length, msg.data);
    }
}

Which platform are you running on: Remix IDE
How to reproduce the issue(abstract code):

var receiver = new Receiver();
var caller2 = new Caller2(receiver.address);
var caller = new Caller(caller2.address);
caller.forward();

What was the result of the issue(events emitted):

Debug(4, '0xd264e05e')
DebugReceived(32, '0xd264e05e00000000000000000000000000000000000000000000000000000000')

What the expected behavior is:

Debug(4, '0xd264e05e')
DebugReceived(4, '0xd264e05e')

Example in mainnet https://etherscan.io/tx/0x994098c4d8166baa361729b48470b44e700bf4ccfb214ebc81cf872dc3d64d15 (check Parity trace)

Contract that cannot receive calls from multisigs due to this bug:
https://etherscan.io/address/0x1776e1f26f98b1a5df9cd347953a26dd3cb46671#code

Code that fails to process corrupted calls:

    // mitigate short address attack
    modifier onlyPayloadSize(uint numWords) {
        assert(msg.data.length == numWords * 32 + 4);
        _;
    }
@lastperson

This comment has been minimized.

Copy link
Contributor

lastperson commented Sep 12, 2017

Examining the documentation on ABI encoding, I see that this is by design. The question then (I fail to find answer in the documentation) is why it is by design? I see the point for fixed size types, but if in order to find the end of a dynamic type we need to examine it's length anyway, then why do the right padding to a multiple of 32?

@chriseth

This comment has been minimized.

Copy link
Contributor

chriseth commented Sep 12, 2017

I'm sorry, but I still do not understand everything. The fact that a.call(x) pads to multiples of 32 bytes indeed sounds like a bug.

Your statement "in order to find the end of a dynamic type we need to examine it's length" should not be correct, the ABI is designed to ignore the length of the input - that's also why it is not checked by solidity functions.

Could you please describe the observed and the expected encoded data?

@lastperson

This comment has been minimized.

Copy link
Contributor

lastperson commented Sep 12, 2017

Yep, I also thing that a.call(x) padding thing is a bug. As per encoding, here is what I think is reasonable, tough probably hardly implementable to maintain backwards compatibility:

Assuming you have a function function encoding(bytes _a, bytes _b), calling it like encoding('0x11111111', '0x222222222222') will result in the following input:

0x540e773b
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000080
0000000000000000000000000000000000000000000000000000000000000004
1111111100000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000006
2222222222220000000000000000000000000000000000000000000000000000

While I see no reason(probably I just didn't think too much about it) why it cannot be:

0x540e773b
0000000000000000000000000000000000000000000000000000000000000040
0000000000000000000000000000000000000000000000000000000000000064
0000000000000000000000000000000000000000000000000000000000000004
1111111100000000000000000000000000000000000000000000000000000000
00000006222222222222

Anyways, can we deal with a.call(x) thing? :)

@chriseth

This comment has been minimized.

Copy link
Contributor

chriseth commented Sep 13, 2017

You mentioned the ABI :-)

a.call(x) does not do any ABI encoding.

@chriseth

This comment has been minimized.

Copy link
Contributor

chriseth commented Sep 13, 2017

Ok, I had a look at the code again and we actually need padding for a.call(), otherwise a.call(1,2,3) would not work as expected.

The thing is: a.call() is an ancient beast that should not be used. I would recommend using inline assembly for such tasks, since it provides the same security guarantees but does not do any invisible magic.

@lastperson

This comment has been minimized.

Copy link
Contributor

lastperson commented Sep 14, 2017

Example with function encoding() is about ABI encoding, how it is working right now, and how it might work. Not related to a.call(x).

we actually need padding for a.call(), otherwise a.call(1,2,3) would not work as expected.

Can't there be an exception to the case when a single bytes type parameter passed in? Why it works differently when we pass msg.data there?

pragma solidity 0.4.16;
contract Caller {
    address receiver; 
    function Caller(address _rec) {
        receiver = _rec;
    } 
    function forward() {
        receiver.call(msg.data); // not padded, DebugReceived(4, '0xd264e05e')
        bytes memory b = bytes(msg.data);
        receiver.call(b); // padded, DebugReceived(32, '0xd264e05e00000000000000000000000000000000000000000000000000000000')
    }
}
contract Receiver {
    event DebugReceive(uint, bytes);
    function () {
        DebugReceive(msg.data.length, msg.data);
    }
}
@chriseth

This comment has been minimized.

Copy link
Contributor

chriseth commented Sep 14, 2017

Ah, that's interesting! You might want to try changing forward to be external. But in general: call has many weird quirks and I would much rather just remove it from the language than fix all these quirks.

@AlexeyAkhunov

This comment has been minimized.

Copy link

AlexeyAkhunov commented Dec 3, 2017

The fix for this seems to be addition to this line: https://github.com/ethereum/solidity/blob/develop/libsolidity/ast/Types.h#L1022

The original line

bool padArguments() const { return !(m_kind == Kind::SHA3 || m_kind == Kind::SHA256 || m_kind == Kind::RIPEMD160); }

should be extended to (and to include BareDelegateCall and BareCallCode, though I did not test them):

bool padArguments() const { return !(m_kind == Kind::SHA3 || m_kind == Kind::SHA256 || m_kind == Kind::RIPEMD160 || m_kind == Kind::BareCall); }

I have tested the addition using Gnosis Multisig and transfer call with onlyPayloadSize modifier.

Unfortunately, I don't see how to embed this test into solidity repository, because it relies on go-ethereum and its abigen tool.

@axic

This comment has been minimized.

Copy link
Member

axic commented Apr 17, 2018

@chriseth is this still outstanding?

@chriseth

This comment has been minimized.

Copy link
Contributor

chriseth commented Apr 17, 2018

@axic I think so.

My proposal would be to change call to not do packed encoding anymore, but only accept a single bytes parameter and not do any padding. Maybe support a single parameterless overload.

@chriseth chriseth added this to To do in 0.5.0 via automation Apr 17, 2018

@chriseth chriseth moved this from To do to To discuss in 0.5.0 May 14, 2018

@chriseth chriseth moved this from To discuss to Ready to be worked on in 0.5.0 May 16, 2018

@chriseth chriseth moved this from Ready to be worked on to In progress in 0.5.0 May 23, 2018

@axic

This comment has been minimized.

Copy link
Member

axic commented Aug 1, 2018

@chriseth I think we implemented everything here?

@chriseth

This comment has been minimized.

Copy link
Contributor

chriseth commented Aug 1, 2018

Yes!

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