Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: relay old failed messages after migration #111

Merged
merged 31 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
2bd626a
fix: stop incomming messages
hexshire Jul 16, 2024
ddb4aa4
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 17, 2024
aa963ea
fix: comment
hexshire Jul 17, 2024
cccad1f
refactor: remove migrated check from l1 adapter
hexshire Jul 17, 2024
1695ed4
refactor: follow cei pattern
hexshire Jul 17, 2024
fbe4d05
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 23, 2024
fbb0a4e
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 23, 2024
2e9514e
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 23, 2024
5d0a4de
refactor: block update master minter function
hexshire Jul 23, 2024
71fecee
refactor: remove master minter
hexshire Jul 24, 2024
43e52f1
test: integration test
hexshire Jul 24, 2024
e0ff171
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 24, 2024
8133a7a
refactor: use enum for l2 adapter
hexshire Jul 24, 2024
2b9a632
refactor: use internal function to send messages
hexshire Jul 24, 2024
e0596e5
refactor: add spender param to messages
hexshire Jul 24, 2024
4a75bc6
refactor: add sanity check
hexshire Jul 24, 2024
e1e7eae
refactor: receive message logic
hexshire Jul 24, 2024
1e81bfa
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 24, 2024
aa81beb
test: add tests
hexshire Jul 24, 2024
3c143b2
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 24, 2024
1fd4c61
test: integration test after deprecation
hexshire Jul 24, 2024
fce0118
test: integration test after deprecation
hexshire Jul 24, 2024
4f06e80
chore: fix comment
hexshire Jul 25, 2024
3bb8d4b
fix: eip-712 compliance
Jul 25, 2024
a591a95
Revert "fix: eip-712 compliance"
Jul 25, 2024
06486f9
Merge branch 'audit/spearbit' into fix/relay-failed-messages
hexshire Jul 25, 2024
3111a0d
Update src/contracts/L2OpUSDCBridgeAdapter.sol
hexshire Jul 25, 2024
86141df
refactor: readability
hexshire Jul 25, 2024
4cf8ef3
chore: add param name
hexshire Jul 25, 2024
b3fc454
test: rename test case
hexshire Jul 25, 2024
1063e6e
chore: add natspec
hexshire Jul 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 22 additions & 25 deletions src/contracts/L1OpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@
/// @inheritdoc IL1OpUSDCBridgeAdapter
address public burnCaller;

/// @inheritdoc IL1OpUSDCBridgeAdapter
Status public messengerStatus;

/**
* @notice Modifier to check if the sender is the linked adapter through the messenger
*/
Expand Down Expand Up @@ -203,15 +200,7 @@
// Ensure messaging is enabled
if (messengerStatus != Status.Active) revert IOpUSDCBridgeAdapter_MessagingDisabled();

// Transfer the tokens to the contract
IUSDC(USDC).safeTransferFrom(msg.sender, address(this), _amount);

// Send the message to the linked adapter
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_to, _amount)), _minGasLimit
);

emit MessageSent(msg.sender, _to, _amount, MESSENGER, _minGasLimit);
_sendMessage(msg.sender, _to, _amount, _minGasLimit);
}

/**
Expand Down Expand Up @@ -254,24 +243,17 @@
// Mark the nonce as used
userNonces[_signer][_nonce] = true;

// Transfer the tokens to the contract
IUSDC(USDC).safeTransferFrom(_signer, address(this), _amount);

// Send the message to the linked adapter
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_to, _amount)), _minGasLimit
);

emit MessageSent(_signer, _to, _amount, MESSENGER, _minGasLimit);
_sendMessage(_signer, _to, _amount, _minGasLimit);
}

/**
* @notice Receive the message from the other chain and transfer the tokens to the user
* @dev This function should only be called when receiving a message to mint the bridged representation
* @param _user The user to mint the bridged representation for
* @param _amount The amount of tokens to mint
* @dev This function should only be called when receiving a message to transfer the tokens
* @param _user The user to transfer the tokens to
* @param _spender The address that provided the tokens
* @param _amount The amount of tokens to transfer
*/
function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {
function receiveMessage(address _user, address _spender, uint256 _amount) external override onlyLinkedAdapter {

Check warning on line 256 in src/contracts/L1OpUSDCBridgeAdapter.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Variable "_spender" is unused
// Transfer the tokens to the user
try this.attemptTransfer(_user, _amount) {
emit MessageReceived(_user, _amount, MESSENGER);
Expand Down Expand Up @@ -307,4 +289,19 @@
if (msg.sender != address(this)) revert IOpUSDCBridgeAdapter_InvalidSender();
IUSDC(USDC).safeTransfer(_to, _amount);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing NATSPEC here

// Transfer the tokens to the contract
IUSDC(USDC).safeTransferFrom(_from, address(this), _amount);

// Send the message to the linked adapter
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_to, _from, _amount)), _minGasLimit
);

emit MessageSent(_from, _to, _amount, MESSENGER, _minGasLimit);
}
}
74 changes: 42 additions & 32 deletions src/contracts/L2OpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,12 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
bytes4 internal constant _UPGRADE_TO_SELECTOR = 0x3659cfe6;
///@notice `upgradeToAndCall(address,bytes)` USDC function selector
bytes4 internal constant _UPGRADE_TO_AND_CALL_SELECTOR = 0x4f1ef286;
///@notice `updateMasterMinter(address)` USDC function selector
bytes4 internal constant _UPDATE_MASTER_MINTER_SELECTOR = 0xaa20e1e4;

/// @inheritdoc IL2OpUSDCBridgeAdapter
FallbackProxyAdmin public immutable FALLBACK_PROXY_ADMIN;

/// @inheritdoc IL2OpUSDCBridgeAdapter
bool public isMessagingDisabled;

/// @inheritdoc IL2OpUSDCBridgeAdapter
address public roleCaller;

Expand Down Expand Up @@ -77,14 +76,17 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
* @param _setBurnAmountMinGasLimit Minimum gas limit that the setBurnAmount message can be executed on L1
*/
function receiveMigrateToNative(address _roleCaller, uint32 _setBurnAmountMinGasLimit) external onlyLinkedAdapter {
isMessagingDisabled = true;
messengerStatus = Status.Deprecated;
roleCaller = _roleCaller;

// We need to do totalSupply + blacklistedFunds
// Because on `receiveMessage` mint would fail causing the totalSupply to not increase
// But the native token is still locked on L1
uint256 _burnAmount = IUSDC(USDC).totalSupply();

// Remove the L2 Adapter as a minter
IUSDC(USDC).removeMinter(address(this));

ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IL1OpUSDCBridgeAdapter.setBurnAmount, (_burnAmount)), _setBurnAmountMinGasLimit
);
Expand Down Expand Up @@ -115,7 +117,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
* @notice Receive the stop messaging message from the linked adapter and stop outgoing messages
*/
function receiveStopMessaging() external onlyLinkedAdapter {
isMessagingDisabled = true;
messengerStatus = Status.Paused;

emit MessagingStopped(MESSENGER);
}
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


emit MessagingResumed(MESSENGER);
}
Expand All @@ -145,19 +147,9 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
if (IUSDC(USDC).isBlacklisted(_to)) revert IOpUSDCBridgeAdapter_BlacklistedAddress();

// Ensure messaging is enabled
if (isMessagingDisabled) revert IOpUSDCBridgeAdapter_MessagingDisabled();

IUSDC(USDC).safeTransferFrom(msg.sender, address(this), _amount);

// Burn the tokens
IUSDC(USDC).burn(_amount);

// Send the message to the linked adapter
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_to, _amount)), _minGasLimit
);
if (messengerStatus != Status.Active) revert IOpUSDCBridgeAdapter_MessagingDisabled();

emit MessageSent(msg.sender, _to, _amount, MESSENGER, _minGasLimit);
_sendMessage(msg.sender, _to, _amount, _minGasLimit);
}

/**
Expand All @@ -183,7 +175,7 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
if (IUSDC(USDC).isBlacklisted(_to)) revert IOpUSDCBridgeAdapter_BlacklistedAddress();

// Ensure messaging is enabled
if (isMessagingDisabled) revert IOpUSDCBridgeAdapter_MessagingDisabled();
if (messengerStatus != Status.Active) revert IOpUSDCBridgeAdapter_MessagingDisabled();

// Ensure the nonce has not already been used
if (userNonces[_signer][_nonce]) revert IOpUSDCBridgeAdapter_InvalidNonce();
Expand All @@ -200,26 +192,24 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
// Mark the nonce as used
userNonces[_signer][_nonce] = true;

IUSDC(USDC).safeTransferFrom(_signer, address(this), _amount);

// Burn the tokens
IUSDC(USDC).burn(_amount);

// Send the message to the linked adapter
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_to, _amount)), _minGasLimit
);

emit MessageSent(_signer, _to, _amount, MESSENGER, _minGasLimit);
_sendMessage(_signer, _to, _amount, _minGasLimit);
}

/**
* @notice Receive the message from the other chain and mint the bridged representation for the user
* @dev This function should only be called when receiving a message to mint the bridged representation
* @param _user The user to mint the bridged representation for
* @param _spender The address that provided the tokens
* @param _amount The amount of tokens to mint
*/
function receiveMessage(address _user, uint256 _amount) external override onlyLinkedAdapter {
function receiveMessage(address _user, address _spender, uint256 _amount) external override onlyLinkedAdapter {
if (messengerStatus == Status.Deprecated) {
//Return the funds to the user if the contract is deprecated
// The user will need to relay the message manually with a higher gas limit
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_spender, _spender, _amount)), 0
);
}
Copy link
Contributor

@excaliborr excaliborr Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Mint the tokens to the user
try IUSDC(USDC).mint(_user, _amount) {
emit MessageReceived(_user, _amount, MESSENGER);
Expand Down Expand Up @@ -263,7 +253,10 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
bytes4 _selector = bytes4(_data);
bool _success;

if (_selector == _TRANSFER_OWNERSHIP_SELECTOR || _selector == _CHANGE_ADMIN_SELECTOR) {
if (
_selector == _TRANSFER_OWNERSHIP_SELECTOR || _selector == _CHANGE_ADMIN_SELECTOR
|| _selector == _UPDATE_MASTER_MINTER_SELECTOR
) {
revert IOpUSDCBridgeAdapter_ForbiddenTransaction();
} else if (_selector == _UPGRADE_TO_SELECTOR || _selector == _UPGRADE_TO_AND_CALL_SELECTOR) {
(_success,) = address(FALLBACK_PROXY_ADMIN).call(_data);
Expand All @@ -277,4 +270,21 @@ contract L2OpUSDCBridgeAdapter is IL2OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {

emit USDCFunctionSent(_selector);
}

/*///////////////////////////////////////////////////////////////
INTERNAL FUNCTIONS
///////////////////////////////////////////////////////////////*/
function _sendMessage(address _from, address _to, uint256 _amount, uint32 _minGasLimit) internal {
IUSDC(USDC).safeTransferFrom(_from, address(this), _amount);

// Burn the tokens
IUSDC(USDC).burn(_amount);

// Send the message to the linked adapter
ICrossDomainMessenger(MESSENGER).sendMessage(
LINKED_ADAPTER, abi.encodeCall(IOpUSDCBridgeAdapter.receiveMessage, (_to, _from, _amount)), _minGasLimit
);

emit MessageSent(_from, _to, _amount, MESSENGER, _minGasLimit);
}
}
6 changes: 5 additions & 1 deletion src/contracts/universal/OpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ abstract contract OpUSDCBridgeAdapter is IOpUSDCBridgeAdapter, Ownable {
/// @inheritdoc IOpUSDCBridgeAdapter
address public immutable MESSENGER;

/// @inheritdoc IOpUSDCBridgeAdapter
Status public messengerStatus;

/// @inheritdoc IOpUSDCBridgeAdapter
mapping(address _user => mapping(uint256 _nonce => bool _used)) public userNonces;

Expand Down Expand Up @@ -75,9 +78,10 @@ abstract contract OpUSDCBridgeAdapter is IOpUSDCBridgeAdapter, Ownable {
* @notice Receive the message from the other chain and mint the bridged representation for the user
* @dev This function should only be called when receiving a message to mint the bridged representation
* @param _user The user to mint the bridged representation for
* @param _spender The address that provided the tokens
* @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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* @notice Withdraws the blacklisted funds from the contract if they get unblacklisted
Expand Down
24 changes: 0 additions & 24 deletions src/interfaces/IL1OpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,6 @@
pragma solidity 0.8.25;

interface IL1OpUSDCBridgeAdapter {
/*///////////////////////////////////////////////////////////////
ENUMS
///////////////////////////////////////////////////////////////*/

/**
* @notice The status of an L1 Messenger
* @param Active The messenger is active
* @param Paused The messenger is paused
* @param Upgrading The messenger is upgrading
* @param Deprecated The messenger is deprecated
*/
enum Status {
Active,
Paused,
Upgrading,
Deprecated
}

/*///////////////////////////////////////////////////////////////
EVENTS
///////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -98,10 +80,4 @@ interface IL1OpUSDCBridgeAdapter {
* @return _burnCaller The address of the burn caller
*/
function burnCaller() external view returns (address _burnCaller);

/**
* @notice Fetches the status of the messenger
* @return _status The status of the messenger
*/
function messengerStatus() external view returns (Status _status);
}
5 changes: 0 additions & 5 deletions src/interfaces/IL2OpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,6 @@ interface IL2OpUSDCBridgeAdapter {
/*///////////////////////////////////////////////////////////////
VARIABLES
///////////////////////////////////////////////////////////////*/
/**
* @notice Fetches whether messaging is disabled
* @return _isMessagingDisabled Whether messaging is disabled
*/
function isMessagingDisabled() external view returns (bool _isMessagingDisabled);

/**
* @notice Fetches the address of the role caller
Expand Down
31 changes: 30 additions & 1 deletion src/interfaces/IOpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,23 @@
pragma solidity 0.8.25;

interface IOpUSDCBridgeAdapter {
/*///////////////////////////////////////////////////////////////
ENUMS
///////////////////////////////////////////////////////////////*/

/**
* @notice The status of an L1 Messenger
* @param Active The messenger is active
* @param Paused The messenger is paused
* @param Upgrading The messenger is upgrading
* @param Deprecated The messenger is deprecated
*/
enum Status {
Active,
Paused,
Upgrading,
Deprecated
}
/*///////////////////////////////////////////////////////////////
EVENTS
///////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -127,6 +144,11 @@ interface IOpUSDCBridgeAdapter {
*/
error IOpUSDCBridgeAdapter_BlacklistedAddress();

/**
* @notice Error when bridgedUSDC has already been migrated to native USDC
*/
error IOpUSDCBridgeAdapter_Migrated();

/*///////////////////////////////////////////////////////////////
LOGIC
///////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -162,9 +184,10 @@ interface IOpUSDCBridgeAdapter {
* @notice Receive the message from the other chain and mint the bridged representation for the user
* @dev This function should only be called when receiving a message to mint the bridged representation
* @param _user The user to mint the bridged representation for
* @param _spender The address that provided the tokens
* @param _amount The amount of tokens to mint
*/
function receiveMessage(address _user, uint256 _amount) external;
function receiveMessage(address _user, address _spender, uint256 _amount) external;

/**
* @notice Withdraws the blacklisted funds from the contract if they get unblacklisted
Expand Down Expand Up @@ -203,6 +226,12 @@ interface IOpUSDCBridgeAdapter {
// solhint-disable-next-line func-name-mixedcase
function MESSENGER() external view returns (address _messenger);

/**
* @notice Fetches the status of the messenger
* @return _status The status of the messenger
*/
function messengerStatus() external view returns (Status _status);

/**
* @notice Returns the nonce of a given user to avoid replay attacks
* @param _user The user to check for
Expand Down
7 changes: 7 additions & 0 deletions src/interfaces/external/IUSDC.sol
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ interface IUSDC is IERC20 {
*/
function configureMinter(address _minter, uint256 _minterAllowedAmount) external returns (bool _result);

/**
* @notice Removes a minter.
* @param _minter The address of the minter to remove.
* @return _result True if the operation was successful.
*/
function removeMinter(address _minter) external returns (bool _result);

/**
* @notice Updates the master minter address.
* @param _newMasterMinter The address of the new master minter.
Expand Down
2 changes: 1 addition & 1 deletion test/integration/IntegrationBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ contract IntegrationBase is Helpers {
address(l2Adapter),
_ZERO_VALUE,
_minGasLimitMint,
abi.encodeWithSignature('receiveMessage(address,uint256)', _user, _supply)
abi.encodeWithSignature('receiveMessage(address,address,uint256)', _user, _user, _supply)
);
}

Expand Down
Loading