Multi AMB requests in one transaction

[k1rill-fedoseev]

Resolves #307

[akolotov]

Thanks for the improvement!

I have few remarks:

1. Please add description headers to the methods you are changing.
2. Since we have adopters already that developed and deployed their extensions for AMB (ETH-POA and ETH-xDAI) it seems that we need to start supporting two options to call requireToPassMessage from the mediator contracts.
3. We need to think of the procedure to upgrade existing bridges - how the contracts could be upgraded as so the previous requests to pass messages cannot be replayed.

[k1rill-fedoseev]

Regarding 2.

How we should distinguish these two functions? We cannot use both:

• function requireToPassMessage(address, bytes, uint256) external returns (bytes32)
• function requireToPassMessage(address, bytes, uint256) external

, they have the same signature.

The old version of mediators can successfully call new version of requireToPassMessage by simply ignoring its output. However, there are problems with two other functions: transactionHash() and failedMessageDataHash(bytes32 _txHash). If we remove them, old mediators logic will definitely break. In case our plan is to support old versions of mediators too, I propose to do the following:

• transactionHash() can return the same value, as messageId() does. It won't be a real transaction hash anymore, but the mediator contract won't notice any difference. In case the mediator emits some events that include a returned value of transactionHash(), off-chain logic for handling such events should be updated.
• failedMessageDataHash(bytes32) can be kept, as it is implemented now. And the AMB contract should continue to save dataHash for each failed message, even it is not needed anymore for the new mediators.

If we do so, then the old version of mediator still will be able to do something like:

function requestFailedMessageFix(bytes32 _txHash) external {
  require(!bridgeContract().messageCallStatus(_txHash));
  require(bridgeContract().failedMessageReceiver(_txHash) == address(this));
  require(bridgeContract().failedMessageSender(_txHash) == mediatorContractOnOtherSide());
  bytes32 dataHash = bridgeContract().failedMessageDataHash(_txHash);
  bytes4 methodSelector = this.fixFailedMessage.selector;
  bytes memory data = abi.encodeWithSelector(methodSelector, dataHash);
  bridgeContract().requireToPassMessage(mediatorContractOnOtherSide(), data, requestGasLimit());
}

And the new version mediator will be able to do something like:

function requestFailedMessageFix(bytes32 messageId) external {
  require(!bridgeContract().messageCallStatus(messageId));
  require(bridgeContract().failedMessageReceiver(messageId) == address(this));
  require(bridgeContract().failedMessageSender(messageId) == mediatorContractOnOtherSide());
  bytes4 methodSelector = this.fixFailedMessage.selector;
  bytes memory data = abi.encodeWithSelector(methodSelector, messageId);
  bridgeContract().requireToPassMessage(mediatorContractOnOtherSide(), data, requestGasLimit());
}

Regarding 3.

Replay attack won't be possible, since the message structure and processing logic didn't change.
For foreign contract, messageId has the same format and position in the encoded message, as txHash, so if someone will try to relay old already handled transaction, contract will treat txHash from this message as a messageId, but the require(!relayedMessages(messageId)); will revert the execution.
For home contract the situation is similar, if some validator will try to executeAffirmation on old message, the execution will be reverted, since the provided msgHash was already marked as processed.

[akolotov]

@k1rill-fedoseev

I agree on the item 2. Thanks.

But for the item 3 we need to think more. Since the tx hash now is not the part of the message could it compromise security? In other words, if the message id is unique enough? E.g. it could be the same in different chains if the bridge contract was deployed from the same account with the same number of transaction (nonce). Is a similar property applicable for the tx hashes and they also could be non-unique?

[k1rill-fedoseev]

This PR will eventually close #373 also. The existing bridge does not have any failed messages, so the support of previously used approach is not needed.

[akolotov]

Let's prepare the changes for the oracle as well. We can trail the upgrade then on the bridge between Kovan and Sokol.

[akolotov]

As per offline discussion please consider to extend the message sending through the bridge as following:

version[4] -----|
address[20]     | message id
nonce[8]   -----|
chainId[32]
sender[20]
executor[20]
gas[4] 
dataType[1]
data[…]

[akolotov]

Another set of the comments

[akolotov]

Well, let's leave this as is now and address in a separate issue. We need to commit the changes for this new functionality in short time.

[akolotov]

Consider suggestions to use integers for the chain ids rather than set of bytes.

[akolotov]

Well, I think you need to add a comment before the assembly part saying that "in order to safe gas the header is packed from the end"

[akolotov]

I would also add a comment here "it does not make sense to keep the bridge Id in a storage since the operation to read it will take 800 gas whereas the operations to get sha3 plus to combine the integer and the address into the byte blob will consume less"

[akolotov]

Suggested change

	// can be ommitted for dataType 0x00, since header is already initialized with zeros
	// can be ommitted for dataType 0x00, since header is already initialized with zeros after the previous mstore operation

[k1rill-fedoseev]

In case chainId is 32 bytes, I think that this zero will come from header = new bytes(79 + sourceChainIdLength + destinationChainIdLength); line. This bytes blob should be allocated from the free memory pointer at 0x40, so it will be pre-filled with zeros.

Am I right?

[akolotov]

I am not sure that it is pre-filled with zeros. Did you look at the decompiled solidity code to confirm this? Because, in general, the solidity considerations will work even for the next code to allocate a new set of bytes in the memory:

assembly {
    new_ptr := mload(0x40)
    mstore(0x40, add(ptr, and(add(add(sizeOfNewDataBlob, 0x20), 0x1f), not(0x1f))))
}

As you can see it does not assume that the free memory is zeroed.

[k1rill-fedoseev]

Ok, then I think it makes sense to uncomment this mstore operation, it should not consume a lot of gas.