From 0cc3cf3bef63c4b4c9c52ca2108c5e81e259a6d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Fri, 5 Apr 2024 10:29:36 +0200 Subject: [PATCH 1/8] Add check not to supply less gas than initially specified to be required --- packages/protocol/contracts/bridge/Bridge.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 1b28f3a4c74..b876e0dc7d1 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -346,6 +346,12 @@ contract Bridge is EssentialContract, IBridge { if (_message.gasLimit == 0 || _isLastAttempt) { if (msg.sender != _message.destOwner) revert B_PERMISSION_DENIED(); } + // We do not need to check _message.gasLimit is bigger than (gasleft() * 63) >> 6), simply + // because user can bump the gasleft and it would cause no harm as message status would not + // change anyways. + if (_message.gasLimit != 0 && _message.gasLimit > gasleft()) { + revert B_NOT_ENOUGH_GASLEFT(); + } bytes32 msgHash = hashMessage(_message); if (messageStatus[msgHash] != Status.RETRIABLE) { From 7564c6cd98956fcbc203d97c5b07e0dd021e3e7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Fri, 5 Apr 2024 10:45:26 +0200 Subject: [PATCH 2/8] move check lower and add comment --- packages/protocol/contracts/bridge/Bridge.sol | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index b876e0dc7d1..b34da86bd51 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -346,18 +346,19 @@ contract Bridge is EssentialContract, IBridge { if (_message.gasLimit == 0 || _isLastAttempt) { if (msg.sender != _message.destOwner) revert B_PERMISSION_DENIED(); } - // We do not need to check _message.gasLimit is bigger than (gasleft() * 63) >> 6), simply - // because user can bump the gasleft and it would cause no harm as message status would not - // change anyways. - if (_message.gasLimit != 0 && _message.gasLimit > gasleft()) { - revert B_NOT_ENOUGH_GASLEFT(); - } bytes32 msgHash = hashMessage(_message); if (messageStatus[msgHash] != Status.RETRIABLE) { revert B_NON_RETRIABLE(); } + // We check _message.gasLimit > gasleft() to make sure we not only need to bridge invocation + // call to succeeed, we also need it to succeed with a gas limit no smaller than the + // message's gasLimit. + if (_message.gasLimit != 0 && _message.gasLimit > gasleft()) { + revert B_NOT_ENOUGH_GASLEFT(); + } + // Attempt to invoke the messageCall. if (_invokeMessageCall(_message, msgHash, gasleft())) { _updateMessageStatus(msgHash, Status.DONE); From b58f76a679f71bf039496e0ae1c7d34cdf8a6267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Fri, 5 Apr 2024 11:26:06 +0200 Subject: [PATCH 3/8] add destOwner gas limit check --- packages/protocol/contracts/bridge/Bridge.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index b34da86bd51..574cc7c06e2 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -355,7 +355,12 @@ contract Bridge is EssentialContract, IBridge { // We check _message.gasLimit > gasleft() to make sure we not only need to bridge invocation // call to succeeed, we also need it to succeed with a gas limit no smaller than the // message's gasLimit. - if (_message.gasLimit != 0 && _message.gasLimit > gasleft()) { + // Also if caller is not the destOwner, we need to be sure there is enough gas for rest of + // the code to finish execution. + if ( + (_message.gasLimit != 0 && _message.gasLimit > gasleft()) + || (msg.sender != _message.destOwner && _message.gasLimit > (gasleft() * 63) >> 6) + ) { revert B_NOT_ENOUGH_GASLEFT(); } From d8e6be5dae70adda4cb3f554ccc0971072526291 Mon Sep 17 00:00:00 2001 From: Daniel Wang <99078276+dantaik@users.noreply.github.com> Date: Fri, 5 Apr 2024 17:43:07 +0800 Subject: [PATCH 4/8] Update packages/protocol/contracts/bridge/Bridge.sol --- packages/protocol/contracts/bridge/Bridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 574cc7c06e2..4e220acf89e 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -353,7 +353,7 @@ contract Bridge is EssentialContract, IBridge { } // We check _message.gasLimit > gasleft() to make sure we not only need to bridge invocation - // call to succeeed, we also need it to succeed with a gas limit no smaller than the + // call to succeed, we also need it to succeed with a gas limit no smaller than the // message's gasLimit. // Also if caller is not the destOwner, we need to be sure there is enough gas for rest of // the code to finish execution. From b87cef98a9d01bfe685a501c30f13c1858199caf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Fri, 5 Apr 2024 11:52:14 +0200 Subject: [PATCH 5/8] simplify check --- packages/protocol/contracts/bridge/Bridge.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 574cc7c06e2..b416e7e771e 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -355,11 +355,11 @@ contract Bridge is EssentialContract, IBridge { // We check _message.gasLimit > gasleft() to make sure we not only need to bridge invocation // call to succeeed, we also need it to succeed with a gas limit no smaller than the // message's gasLimit. - // Also if caller is not the destOwner, we need to be sure there is enough gas for rest of - // the code to finish execution. if ( - (_message.gasLimit != 0 && _message.gasLimit > gasleft()) - || (msg.sender != _message.destOwner && _message.gasLimit > (gasleft() * 63) >> 6) + ( + _message.gasLimit != 0 && msg.sender != _message.destOwner + && _message.gasLimit > (gasleft() * 63) >> 6 + ) ) { revert B_NOT_ENOUGH_GASLEFT(); } From 8f8af774799e36eeaefef5d64f5c8818c5597f59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keszey=20D=C3=A1niel?= Date: Fri, 5 Apr 2024 11:52:50 +0200 Subject: [PATCH 6/8] typo --- packages/protocol/contracts/bridge/Bridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index b416e7e771e..05ae1693344 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -353,7 +353,7 @@ contract Bridge is EssentialContract, IBridge { } // We check _message.gasLimit > gasleft() to make sure we not only need to bridge invocation - // call to succeeed, we also need it to succeed with a gas limit no smaller than the + // call to succeed, we also need it to succeed with a gas limit no smaller than the // message's gasLimit. if ( ( From 7e00308d9e8748f64c283be598be89029da158a3 Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Fri, 5 Apr 2024 18:03:46 +0800 Subject: [PATCH 7/8] Update Bridge.sol --- packages/protocol/contracts/bridge/Bridge.sol | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 05ae1693344..956fe401bfb 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -356,10 +356,8 @@ contract Bridge is EssentialContract, IBridge { // call to succeed, we also need it to succeed with a gas limit no smaller than the // message's gasLimit. if ( - ( - _message.gasLimit != 0 && msg.sender != _message.destOwner - && _message.gasLimit > (gasleft() * 63) >> 6 - ) + _message.gasLimit != 0 && msg.sender != _message.destOwner + && _message.gasLimit > (gasleft() * 63) >> 6 ) { revert B_NOT_ENOUGH_GASLEFT(); } From 56b5529d65478504cd4c1e878c210d4c9b8b88d1 Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Fri, 5 Apr 2024 18:05:30 +0800 Subject: [PATCH 8/8] Update Bridge.sol --- packages/protocol/contracts/bridge/Bridge.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 956fe401bfb..cab2afbe4b5 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -352,9 +352,9 @@ contract Bridge is EssentialContract, IBridge { revert B_NON_RETRIABLE(); } - // We check _message.gasLimit > gasleft() to make sure we not only need to bridge invocation - // call to succeed, we also need it to succeed with a gas limit no smaller than the - // message's gasLimit. + // We check gasleft() against _message.gasLimit to make sure we not only need to bridge + // invocation call to succeed, we also need it to succeed with a gas limit no smaller than + // the message's gasLimit. if ( _message.gasLimit != 0 && msg.sender != _message.destOwner && _message.gasLimit > (gasleft() * 63) >> 6