refactor: blacklisted funds logic #125

hexshire · 2024-07-26T16:15:23Z

No description provided.

src/contracts/L1OpUSDCBridgeAdapter.sol

excaliborr · 2024-07-26T16:41:03Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ ) external override onlyLinkedAdapter {
+ if (messengerStatus != Status.Deprecated) revert IOpUSDCBridgeAdapter_NotMigrated();
+
+ IUSDC(USDC).safeTransfer(_spender, _amount);


Add a comment that if the _spender is still blacklisted, the chain operator will be forced to replay this message, as the transfer will fail

In this case _spender was not blacklisted, the user on l2 was the problem would be if the _spender is blacklisted after it sends the message to l2 to a target that is also blacklisted.

😵‍💫

wdym? This is only to cover the scenario because the blacklist is not atomic, and we dont care if _spender is blacklisted or not on L1, if hes ever un-blacklisted he just replays this right? The blacklist on l1 != the blacklist on l2 and shouldnt assume they match

Add a comment that if the _spender is still blacklisted, the chain operator will be forced to replay this message, as the transfer will fail

need to still add this comment

Sorry, It was added in the natspec, above the function.

src/contracts/L2OpUSDCBridgeAdapter.sol

Co-authored-by: excaliborr <124819095+excaliborr@users.noreply.github.com>

…land/opUSDC into fix/refund-blacklisted-funds

src/contracts/L1OpUSDCBridgeAdapter.sol

src/interfaces/IOpUSDCBridgeAdapter.sol

src/contracts/L1OpUSDCBridgeAdapter.sol

0xDiscotech · 2024-07-26T21:11:19Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 }
 }

+ /**
+ * @notice Withdraws the blacklisted funds from L2


Would be better to explain a bit more here what the function does and what's its purpose is

Btw, why did you create this function? Isn't cleaner/simpler to handle this logic on receiveMessage only if the contract has been migrated?
Maybe I'm missing something

I think that way is also more consistent with receiveMessage on the l2 adapter

This is intentional as receiveMessage will not revert if the user is still blacklisted on L1, but will actually keep incrementing the amount of blacklistAmount as the try block will always fail if they are still blacklisted.

function receiveMessage(address _user, address _spender, uint256 _amount) external override onlyLinkedAdapter { // Transfer the tokens to the user try this.attemptTransfer(_user, _amount) { // Fails cause user is still blacklisted emit MessageReceived(_user, _amount, MESSENGER); } catch { userBlacklistedFunds[_user] += _amount; // Increments users funds emit MessageFailed(_user, _amount, MESSENGER); } }

They can do this an infinite amount of times because L2 will never know if the address on L1 is blacklisted, as the blacklist is not atomic.

This extra fn was added to handle this edgecase

src/contracts/L2OpUSDCBridgeAdapter.sol

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

excaliborr · 2024-07-29T11:53:49Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 */
- // solhint-disable-next-line no-unused-vars
 function receiveMessage(address _user, address _spender, uint256 _amount) external override onlyLinkedAdapter {
 // Transfer the tokens to the user
 try this.attemptTransfer(_user, _amount) {
 emit MessageReceived(_user, _amount, MESSENGER);


can we add _spender into the MessageReceived event as well

hexshire · 2024-07-29T12:47:45Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 }
 }

+ /**
+ * @notice Withdraws the blacklisted funds from L2 once the adapter is deprecated
+ * @dev If the _spender is still blacklisted, the chain operator will be forced to


@excaliborr

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

src/contracts/L1OpUSDCBridgeAdapter.sol

+ * @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
+ * @dev If the mint fails the funds might be recovered by calling withdrawBlacklistedFunds if the user
+ * is ever unblacklisted
+ * @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


Suggested change

* @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

* @dev If the mint fails the funds might be recovered by calling withdrawBlacklistedFunds if the user

* is ever unblacklisted

* @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

* @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 transfer the tokens

* @dev If the transfer fails the funds might be recovered by calling withdrawBlacklistedFunds

* @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

0xDiscotech · 2024-07-29T14:47:28Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 } catch {
 blacklistedFundsDetails[_spender][_user] += _amount;
 emit MessageFailed(_spender, _user, _amount, MESSENGER);
 }
 }

 /**
- * @notice Withdraws the blacklisted funds from L2
+ * @notice Receives a message from L2 if the adapter is deprecated are a user is withdrawing blacklisted funds


Suggested change

* @notice Receives a message from L2 if the adapter is deprecated are a user is withdrawing blacklisted funds

* @notice Receives a message from L2 if the adapter is deprecated and a user is withdrawing blacklisted funds

0xDiscotech · 2024-07-29T14:48:22Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 } catch {
 blacklistedFundsDetails[_spender][_user] += _amount;
 emit MessageFailed(_spender, _user, _amount, MESSENGER);
 }
 }

 /**
- * @notice Withdraws the blacklisted funds from L2
+ * @notice Receives a message from L2 if the adapter is deprecated are a user is withdrawing blacklisted funds
+ * @dev If the _spender is still blacklisted, the chain operator will be forced to replay this message


Why only the chain operator? The user could be able to replay it as well

Yea your correct just mis worded, will fix

0xDiscotech · 2024-07-29T14:48:35Z

src/contracts/L1OpUSDCBridgeAdapter.sol

 if (messengerStatus != Status.Deprecated) revert IOpUSDCBridgeAdapter_NotMigrated();

+ // If the spender is still blacklisted, the chain operator will be forced to replay this message


Same question here

0xDiscotech · 2024-07-29T14:49:12Z

src/interfaces/IL1OpUSDCBridgeAdapter.sol

@@ -66,7 +66,9 @@ interface IL1OpUSDCBridgeAdapter {
 function resumeMessaging(uint32 _minGasLimit) external;

 /**
- * @notice Withdraws the blacklisted funds from L2
+ * @notice Withdraws the blacklisted funds from L2 once the adapter is deprecated
+ * @dev If the _spender is still blacklisted, the chain operator will be forced to


Same q here

0xDiscotech · 2024-07-29T14:50:21Z

src/contracts/L2OpUSDCBridgeAdapter.sol

+ * @dev If the mint fails the funds might be recovered by calling withdrawBlacklistedFunds if the user
+ * is ever unblacklisted


We are handling the more than one line NATSPECs like this:

Suggested change

* @dev If the mint fails the funds might be recovered by calling withdrawBlacklistedFunds if the user

* is ever unblacklisted

* @dev If the mint fails the funds might be recovered by calling withdrawBlacklistedFunds if the user

* is ever unblacklisted

0xDiscotech · 2024-07-29T15:31:04Z

src/contracts/L1OpUSDCBridgeAdapter.sol

@@ -262,8 +262,7 @@ contract L1OpUSDCBridgeAdapter is IL1OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
 /**
 * @notice Receive the message from the other chain and mint the bridged representation for the user


This keep saying mint instead of transfer

Suggested change

* @notice Receive the message from the other chain and mint the bridged representation for the user

* @notice Receive the message from the other chain and mint the bridged representation for the user

refactor: blacklisted funds

306f363

excaliborr reviewed Jul 26, 2024

View reviewed changes

src/contracts/L1OpUSDCBridgeAdapter.sol Show resolved Hide resolved

hexshire added 2 commits July 26, 2024 13:23

Merge branch 'audit/spearbit' into fix/refund-blacklisted-funds

d04bba3

chore: add natspec

997f814

excaliborr reviewed Jul 26, 2024

View reviewed changes

src/contracts/L2OpUSDCBridgeAdapter.sol Show resolved Hide resolved

excaliborr reviewed Jul 26, 2024

View reviewed changes

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

hexshire and others added 5 commits July 26, 2024 15:37

test: l1 adapter test

7190487

chore: fix natspec

062aa6f

Co-authored-by: excaliborr <124819095+excaliborr@users.noreply.github.com>

Update src/contracts/L2OpUSDCBridgeAdapter.sol

e3a0763

Co-authored-by: excaliborr <124819095+excaliborr@users.noreply.github.com>

test: l2 adapter tests

290183b

Merge branch 'fix/refund-blacklisted-funds' of github.com:defi-wonder…

87b3137

…land/opUSDC into fix/refund-blacklisted-funds

0xDiscotech reviewed Jul 26, 2024

View reviewed changes

0xDiscotech and others added 6 commits July 26, 2024 19:11

feat: add 2 more unit tests on the l2 adapter (#126)

f3e1773

chore: fix natspec

1873665

chore: fix natspec

9ca0155

Update src/contracts/L1OpUSDCBridgeAdapter.sol

b456f8c

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

chore: fix natspec

3e7bb02

test: integration test recover funds after migration

04a922b

hexshire self-assigned this Jul 28, 2024

excaliborr reviewed Jul 29, 2024

View reviewed changes

fix: event

25fc9ca

hexshire commented Jul 29, 2024

View reviewed changes

fix: lint

04297cf

excaliborr force-pushed the fix/refund-blacklisted-funds branch from a85974f to 04297cf Compare July 29, 2024 12:58

Merge branch 'audit/spearbit' into fix/refund-blacklisted-funds

e05b8ad

excaliborr approved these changes Jul 29, 2024

View reviewed changes

0xDiscotech reviewed Jul 29, 2024

View reviewed changes

fix: lint

b50aa35

0xDiscotech reviewed Jul 29, 2024

View reviewed changes

excaliborr added 3 commits July 29, 2024 11:57

fix: comment

09c1ae1

fix: comment

99d4b8b

fix: natspec

a5f7ce0

0xDiscotech approved these changes Jul 29, 2024

View reviewed changes

excaliborr merged commit 3bf97c5 into audit/spearbit Jul 29, 2024
4 checks passed

excaliborr deleted the fix/refund-blacklisted-funds branch July 29, 2024 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: blacklisted funds logic #125

refactor: blacklisted funds logic #125

hexshire commented Jul 26, 2024

excaliborr Jul 26, 2024 •

edited

Loading

hexshire Jul 26, 2024

hexshire Jul 26, 2024

excaliborr Jul 26, 2024

excaliborr Jul 29, 2024

hexshire Jul 29, 2024

0xDiscotech Jul 26, 2024

0xDiscotech Jul 26, 2024

0xDiscotech Jul 26, 2024

excaliborr Jul 26, 2024 •

edited

Loading

excaliborr Jul 29, 2024

hexshire Jul 29, 2024

0xDiscotech Jul 29, 2024

0xDiscotech Jul 29, 2024

0xDiscotech Jul 29, 2024

excaliborr Jul 29, 2024

0xDiscotech Jul 29, 2024

0xDiscotech Jul 29, 2024

0xDiscotech Jul 29, 2024

0xDiscotech Jul 29, 2024

	* @notice Receives a message from L2 if the adapter is deprecated are a user is withdrawing blacklisted funds
	* @notice Receives a message from L2 if the adapter is deprecated and a user is withdrawing blacklisted funds

		if (messengerStatus != Status.Deprecated) revert IOpUSDCBridgeAdapter_NotMigrated();

		// If the spender is still blacklisted, the chain operator will be forced to replay this message

		* @dev If the mint fails the funds might be recovered by calling withdrawBlacklistedFunds if the user
		* is ever unblacklisted

		@@ -262,8 +262,7 @@ contract L1OpUSDCBridgeAdapter is IL1OpUSDCBridgeAdapter, OpUSDCBridgeAdapter {
		/**
		* @notice Receive the message from the other chain and mint the bridged representation for the user

refactor: blacklisted funds logic #125

refactor: blacklisted funds logic #125

Conversation

hexshire commented Jul 26, 2024

excaliborr Jul 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

excaliborr Jul 26, 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

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 26, 2024 •

edited

Loading

excaliborr Jul 26, 2024 •

edited

Loading