Unify usage of hasEnoughValidSignatures

[akolotov]

As part of work under AMB feature a new way to transmit signatures was suggested and implemented as the alternative implementation of hasEnoughValidSignatures:

https://github.com/poanetwork/tokenbridge-contracts/blob/c85aeb737bfd684b20b99520bd632ba4b8866583/contracts/libraries/Message.sol#L124-L157

The reason of the suggestion is to achieve more cheaper transactions for the validators: instead of sending arrays _vs, _rs and _ss of the same size the signatures are transferred as a blob which consumes less gas for sending but could require more gas for handling. Tests showed that the high gas cost for nonzero bytes in the calldata (68 gas) makes the new implementation more effective from gas usage point of view.

The EIP-2028 in the Istanbul hardfork reduces the cost of calldata from 68 to 16 gas per non-zero byte. It means that we need to re-examine if the new implementation is still effective for different number of signatures required for full confirmation. If it is still effective it is necessary to change the executeSignatures method in BasicForeignBridge as so the new implementation is used.

[k1rill-fedoseev]

I have done some quick tests on calling two variations of hasEnoughValidSignatures function using ganache-cli. In the table below, you can see the gas usages I have got.

Number of validators/signatures	Constantinople Arrays	Constantinople Blob	Istanbul Arrays	Istanbul Blob
1	49378	48206	50578	49406
2	62971	61618	64771	63418
3	77131	75595	79531	77995
4	91729	90075	94793	93139
5	107085	105249	110749	108913
6	122943	120924	127079	125060
7	139303	137038	144103	141838
8	156293	153910	161693	159310
9	173850	171284	179786	177220
10	191908	189160	198444	195696
15	291012	287287	300548	296823
20	404204	399693	416676	412165
25	531677	526188	547149	541660
30	672982	666577	691710	685305
35	828695	821509	850359	843173
40	998561	990458	1023097	1014994
45	1182580	1173558	1210052	1201030
50	1380622	1370682	1411030	1401090

From this results, seems that sending a blob is always a better approach in both, Istanbul and Constantinople hardforks.

[akolotov]

Cool! Thanks for rapid testing! So, let's change BasicForeignBridge and test carefully possible impact on existing bridges. Will we be able to upgrade the contracts on the new implementation or due to changes in the contract ABI it could make the contracts vulnerable for the replay attack (old confirmations could be re-send from the Home to Foreign)?

[k1rill-fedoseev]

In my understanding, there won't be any vulnerability.
In https://github.com/poanetwork/tokenbridge-contracts/blob/c85aeb737bfd684b20b99520bd632ba4b8866583/contracts/upgradeable_contracts/BasicForeignBridge.sol#L18-L34 we parse a message, take txHash as a part of this message. Then check that this txHash was not processed before. And finally we setRelayedMessages(txHash, true);.
Since, independently of the way of passing signatures, the messages are identical in both cases, bridge won't allow a replay attack on this.

Am I correct?

[akolotov]

Thanks for analysis. So the next step is to migrate to the new approach to pass signatures.