in HomeBridge.withdraw: recipient gets loaded from message with wrong offset and contains wrong data

[paritytech]

in HomeBridge.withdraw: recipient gets loaded from message with wrong offset and contains wrong data

[snd]

@debris can you have a look at this when you have a free moment. thanks

[tomusdrw]

bytes message is always:
32 bytes - length of the content
20 bytes - address
32 bytes - uint
32 bytes - hash
mload(add(message, 0x20)) just skips the length (since we know it anyway)
The error is in:
value := mload(add(message, 0x40)) actually, cause we skip len (32b) and address (32b instead of 20b) unless we expect address to be padded to 32bytes (which seems to be missing in docs).

[snd]

address is currently not padded to 32bytes. see

parity-bridge/bridge/src/bridge/withdraw_confirm.rs

Line 29 in 5940396

result[0..20].copy_from_slice(&withdraw_log.recipient);

[snd]

@tomusdrw thanks for taking a look at this!

i didn't know that the first 32bytes of a bytes represent the length. makes total sense.

what confuses me is that this test

parity-bridge/truffle/test/home.js

Line 85 in b796746

it("should allow correct withdraw", function() {

only works if i modify the assembly to this:

parity-bridge/contracts/bridge.sol

Line 119 in b796746

recipient := mload(add(message, 20))

if i modify the assembly to:

        assembly {
            // skip 32 bytes message length
            recipient := mload(add(message, 32))
            value := mload(add(message, 52))
            hash := mload(add(message, 84))
        }

which i'd expect to be correct

then what gets loaded into recipient is wrong and the test fails at:

parity-bridge/truffle/test/home.js

Line 129 in b796746

assert.equal(recipient_account, result.logs[0].args.recipient, "Event recipient should match recipient in message");

[snd]

i think we need to pad address to 32 bytes since mload seems to always load 32 bytes

[snd]

#61 (comment)

what happens here is probably that bytes 20-32 are all 0 since length is 84.
thus reading recipient from bytes 20-52 (32 bytes starting at 20) reads 12 zero bytes followed by the 20 recipient bytes which get correctly read as the recipient address.
we could leave it like this since length is always 84 but it would probably be cleaner to pad recipient to 32

[tomusdrw]

Right. That's correct. mload loads 32 bytes and just uses last 20 for address.

 // skip 32 bytes message length
            recipient := mload(add(message, 20))
            value := mload(add(message, 52))
            hash := mload(add(message, 84))

That should work, right? We re-use some bytes from length, but it doesn't matter cause they are discarded anyway.

Otherwise we need to pad address to 32-bytes.

[snd]

That should work, right? We re-use some bytes from length, but it doesn't matter cause they are discarded anyway.

yes, that works!

Otherwise we need to pad address to 32-bytes.

that would be a bit clearer imho

[snd]

how should we resolve this?

1. re-use some bytes from length and document it thoroughly
2. pad address to 32-bytes

fine with both. i like 2 a bit more

[tomusdrw]

I prefer the first one to reduce the gas cost by a tiny bit. That said we should heavily test that function to make sure it's correct. I would also add length assertion at the top if it's not there yet, so that a padded address is not accepted.

[snd]

I prefer the first one to reduce the gas cost by a tiny bit. That said we should heavily test that function to make sure it's correct. I would also add length assertion at the top if it's not there yet, so that a padded address is not accepted.

ok!

already added a length assertion