fix: relay old failed messages after migration #111

hexshire · 2024-07-16T19:31:57Z

Based on Cantina comments

src/contracts/L1OpUSDCBridgeAdapter.sol

src/contracts/universal/OpUSDCBridgeAdapter.sol

src/contracts/L2OpUSDCBridgeAdapter.sol

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>

excaliborr · 2024-07-19T11:24:39Z

src/contracts/L2OpUSDCBridgeAdapter.sol

@@ -207,6 +211,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
 * @param _amount The amount of tokens to mint
 */
 function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {
+ if (isMigrated) revert IOpUSDCBridgeAdapter_Migrated();


Was talking a bit more with spearbit about this, and was thinking we could do the following

As L1 should still hold sufficient funds, what if we trigger a receiveMessage on L1, so we send a new message back to L1 to withdraw on the home chain instead of reverting, what do you guys think?

also since the same spearbit ticket has this can you do the following as well in this PR:

Remove the L2 Adapter as a minter in the receiveMigrateToNative

add the updateMasterMinter call as a blacklisted selector in callUsdcTransaction (double check the actual fn name)

ref:
https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/findings/21?severity=medium

https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/findings/4?severity=medium

Could we add an integration test also? Testing this flow:

User replays a failed mint message after migration happened

No amount is minted but instead a message is sent to L1 back so the user can withdraw his funds

@excaliborr

Was talking a bit more with spearbit about this, and was thinking we could do the following

As L1 should still hold sufficient funds, what if we trigger a receiveMessage on L1, so we send a new message back to L1 to withdraw on the home chain instead of reverting, what do you guys think?

To make this work we should avoid burning the balance of the l1adapter, right? The following piece of code should be changed. We should burn only the equivalent to the totalSupply on the L2

uint256 _balanceOf = IUSDC(USDC).balanceOf(address(this)); _burnAmount = _burnAmount > _balanceOf ? _balanceOf : _burnAmount;

And we also need to add the caller in the receiveMessage function and we also need a minGasLimit to return the funds to the right user. I think is too much for this PR.

function receiveMessage(address _user, uint256 _amount, address _caller) external override onlyLinkedAdapter { if (isMigrated) { // Send the message to the linked adapter ICrossDomainMessenger(MESSENGER).sendMessage( LINKED_ADAPTER, abi.encodeWithSignature('receiveMessage(address,uint256)', _caller, _amount), _minGasLimit ); } else { // Mint the tokens to the user try IUSDC(USDC).mint(_user, _amount) { emit MessageReceived(_user, _amount, MESSENGER); } catch { userBlacklistedFunds[_user] += _amount; emit MessageFailed(_user, _amount); } } }

you can just use _user instead of _caller no need for a new param

imo hardcoding a _minGasLimit for this edgecase should be fine as its a super edgecase, I would just it to like 150_000, if for some reason it does fail, they can replay it on L1, you could also try using something like gas() as well to just copy what its being called with now, but this can be inconsistent

Just be sure to document the reason for hardcoding it as well, which is to not add unneccesary overhead in the 99% of messages

wdyt @0xDiscotech

I think we can´t use _user to return the funds because from L1 you are not sending the funds to the msg.sender you are picking a desired address, in order to return the funds we need to know who sent the original message and we also need to set the minGasLimit since the user will relay a receiveMessage but that one will create another receiveMessage that will go to L1 to return the funds.

I agree using _caller will be more consistent as the target address could be a contract which might not exist on L1, so lets add this extra param then.

excaliborr · 2024-07-24T16:46:51Z

src/contracts/L2OpUSDCBridgeAdapter.sol

@@ -125,7 +127,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
 */
 function receiveResumeMessaging() external onlyLinkedAdapter {
 // NOTE: This is safe because this message can only be received when messaging is not deprecated on the L1 messenger
- isMessagingDisabled = false;
+ messengerStatus = Status.Active;


Thinking more about this lets add a sanity check that messengerStatus != Deprecated it doesnt hurt and helps be safe against potential sequencer ordering bugs

excaliborr · 2024-07-24T16:47:52Z

src/contracts/L2OpUSDCBridgeAdapter.sol

+ ICrossDomainMessenger(MESSENGER).sendMessage(
+ LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_spender, _spender, _amount)), 0
+ );
+ }


this needs to be an if..else block, so the try is only executed if the if is false

also add a comment for why we send to spender instead of user

also dont use 0 as its guaranteed to fail, put like 150_000 which should usually pass

excaliborr · 2024-07-25T12:35:06Z

src/contracts/L2OpUSDCBridgeAdapter.sol

+ if (messengerStatus == Status.Deprecated) {
+ // Return the funds to the address where the tokens were sent from
+ ICrossDomainMessenger(MESSENGER).sendMessage(
+ LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_spender, _spender, _amount)), 150_000


Add a comment like

// We send funds to the spender incase the target on L2 is a contract that doesn't exist on L1

This reverts commit 3bb8d4b.

excaliborr · 2024-07-25T14:52:49Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 */
- function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {
+ function receiveMessage(address _user, address, uint256 _amount) external override onlyLinkedAdapter {


I would still call this address _spender, and put in the natspec its unused on L1, but is set to match the universal selector

or have one receiveMessage function per adapter?

But it matches the universal selector this way and we are not adding an unused var. Unless I add it and also add a linter comment to ignore it.

0xDiscotech · 2024-07-25T14:46:48Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 */
- function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {
+ function receiveMessage(address _user, address, uint256 _amount) external override onlyLinkedAdapter {


What's this unused address param for?

Suggested change

function receiveMessage(address _user, address, uint256 _amount) external override onlyLinkedAdapter {

function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {

It is mainly used on L2 and on sendMessage functions.

src/contracts/L2OpUSDCBridgeAdapter.sol

0xDiscotech · 2024-07-25T15:13:47Z

src/contracts/universal/OpUSDCBridgeAdapter.sol

 * @param _amount The amount of tokens to mint
 */
- function receiveMessage(address _user, uint256 _amount) external virtual;
+ function receiveMessage(address _user, address _spender, uint256 _amount) external virtual;


Consider moving this function to each adapter's itnerface, since one needs the _spender and the other one doesn't

0xDiscotech · 2024-07-25T15:14:39Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 */
- function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {
+ function receiveMessage(address _user, address, uint256 _amount) external override onlyLinkedAdapter {


or have one receiveMessage function per adapter?

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>

0xDiscotech · 2024-07-25T15:48:49Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ /*///////////////////////////////////////////////////////////////
+ INTERNAL FUNCTIONS
+ ///////////////////////////////////////////////////////////////*/
+ function _sendMessage(address _from, address _to, uint256 _amount, uint32 _minGasLimit) internal {


Missing NATSPEC here

fix: stop incomming messages

2bd626a

hexshire requested review from excaliborr and 0xDiscotech July 16, 2024 19:31

0xDiscotech reviewed Jul 16, 2024

View reviewed changes

src/contracts/L1OpUSDCBridgeAdapter.sol Outdated Show resolved Hide resolved

excaliborr reviewed Jul 16, 2024

View reviewed changes

src/contracts/L1OpUSDCBridgeAdapter.sol Outdated Show resolved Hide resolved

excaliborr reviewed Jul 16, 2024

View reviewed changes

src/contracts/universal/OpUSDCBridgeAdapter.sol Outdated Show resolved Hide resolved

excaliborr reviewed Jul 16, 2024

View reviewed changes

src/contracts/L2OpUSDCBridgeAdapter.sol Outdated Show resolved Hide resolved

hexshire and others added 4 commits July 16, 2024 23:47

Merge branch 'audit/spearbit' into fix/relay-failed-messages

ddb4aa4

fix: comment

aa963ea

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>

refactor: remove migrated check from l1 adapter

cccad1f

refactor: follow cei pattern

1695ed4

hexshire requested review from excaliborr and 0xDiscotech July 17, 2024 02:58

excaliborr reviewed Jul 19, 2024

View reviewed changes

Merge branch 'audit/spearbit' into fix/relay-failed-messages

fbe4d05

hexshire self-assigned this Jul 23, 2024

hexshire added 9 commits July 23, 2024 15:52

Merge branch 'audit/spearbit' into fix/relay-failed-messages

fbb0a4e

Merge branch 'audit/spearbit' into fix/relay-failed-messages

2e9514e

refactor: block update master minter function

5d0a4de

refactor: remove master minter

71fecee

test: integration test

43e52f1

Merge branch 'audit/spearbit' into fix/relay-failed-messages

e0ff171

refactor: use enum for l2 adapter

8133a7a

refactor: use internal function to send messages

2b9a632

refactor: add spender param to messages

e0596e5

excaliborr reviewed Jul 24, 2024

View reviewed changes

hexshire and others added 3 commits July 24, 2024 14:31

refactor: add sanity check

4a75bc6

refactor: receive message logic

e1e7eae

Merge branch 'audit/spearbit' into fix/relay-failed-messages

1e81bfa

hexshire added 4 commits July 24, 2024 19:35

test: add tests

aa81beb

Merge branch 'audit/spearbit' into fix/relay-failed-messages

3c143b2

test: integration test after deprecation

1fd4c61

test: integration test after deprecation

fce0118

hexshire requested a review from excaliborr July 25, 2024 02:45

excaliborr reviewed Jul 25, 2024

View reviewed changes

hexshire and others added 4 commits July 25, 2024 11:12

chore: fix comment

4f06e80

fix: eip-712 compliance

3bb8d4b

Revert "fix: eip-712 compliance"

a591a95

This reverts commit 3bb8d4b.

Merge branch 'audit/spearbit' into fix/relay-failed-messages

06486f9

excaliborr force-pushed the audit/spearbit branch from a591a95 to 56f49a3 Compare July 25, 2024 14:46

excaliborr reviewed Jul 25, 2024

View reviewed changes

0xDiscotech reviewed Jul 25, 2024

View reviewed changes

hexshire and others added 4 commits July 25, 2024 12:19

Update src/contracts/L2OpUSDCBridgeAdapter.sol

3111a0d

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com>

refactor: readability

86141df

chore: add param name

4cf8ef3

test: rename test case

b3fc454

0xDiscotech reviewed Jul 25, 2024

View reviewed changes

chore: add natspec

1063e6e

0xDiscotech approved these changes Jul 25, 2024

View reviewed changes

excaliborr merged commit 39a260f into audit/spearbit Jul 25, 2024
4 checks passed

excaliborr deleted the fix/relay-failed-messages branch July 25, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: relay old failed messages after migration #111

fix: relay old failed messages after migration #111

hexshire commented Jul 16, 2024 •

edited

Loading

excaliborr Jul 19, 2024

excaliborr Jul 22, 2024

0xDiscotech Jul 22, 2024 •

edited

Loading

hexshire Jul 24, 2024 •

edited

Loading

hexshire Jul 24, 2024 •

edited

Loading

excaliborr Jul 24, 2024

excaliborr Jul 24, 2024 •

edited

Loading

excaliborr Jul 24, 2024

hexshire Jul 24, 2024

excaliborr Jul 24, 2024

excaliborr Jul 24, 2024

excaliborr Jul 24, 2024 •

edited

Loading

excaliborr Jul 24, 2024

excaliborr Jul 24, 2024

excaliborr Jul 25, 2024

excaliborr Jul 25, 2024

0xDiscotech Jul 25, 2024

hexshire Jul 25, 2024 •

edited

Loading

0xDiscotech Jul 25, 2024

hexshire Jul 25, 2024

0xDiscotech Jul 25, 2024

0xDiscotech Jul 25, 2024

0xDiscotech Jul 25, 2024

	function receiveMessage(address _user, address, uint256 _amount) external override onlyLinkedAdapter {
	function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {

fix: relay old failed messages after migration #111

fix: relay old failed messages after migration #111

Conversation

hexshire commented Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDiscotech Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

hexshire Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

hexshire Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

excaliborr Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

excaliborr Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexshire Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexshire commented Jul 16, 2024 •

edited

Loading

0xDiscotech Jul 22, 2024 •

edited

Loading

hexshire Jul 24, 2024 •

edited

Loading

hexshire Jul 24, 2024 •

edited

Loading

excaliborr Jul 24, 2024 •

edited

Loading

excaliborr Jul 24, 2024 •

edited

Loading

hexshire Jul 25, 2024 •

edited

Loading