fix: edgecase where user is blacklisted with an inflight message #105

excaliborr · 2024-07-15T17:55:32Z

🤖 Linear

Closes OPT-XXX

src/contracts/L1OpUSDCBridgeAdapter.sol

src/contracts/L2OpUSDCBridgeAdapter.sol

src/contracts/L1OpUSDCBridgeAdapter.sol

src/contracts/L2OpUSDCBridgeAdapter.sol

0xDiscotech · 2024-07-15T21:17:13Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ function withdrawBlacklistedFunds() external onlyOwner {
+ uint256 _amount = blacklistedFunds;
+ blacklistedFunds = 0;
+
+ IUSDC(USDC).safeTransfer(owner(), _amount);


Why not allowing the owner to set any arbitrary receiver of the funds?

Suggested change

function withdrawBlacklistedFunds() external onlyOwner {

uint256 _amount = blacklistedFunds;

blacklistedFunds = 0;

IUSDC(USDC).safeTransfer(owner(), _amount);

function withdrawBlacklistedFunds(address _to) external onlyOwner {

uint256 _amount = blacklistedFunds;

blacklistedFunds = 0;

IUSDC(USDC).safeTransfer(_to, _amount);

0xDiscotech · 2024-07-15T21:25:01Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ /**
+ * @notice Withdraws the blacklisted funds
+ */
+ function withdrawBlacklistedFunds() external onlyOwner {


For better UX, we shouldn't allow this to be called when the burnAmount is burned. And maybe we should set to 0 the blacklisted amount as well

Why not? Arguably Im not sure we should even allow the blacklisted funds to be withdrawn, maybe we should just burn them, but idk what circles process is for that

bc of this, i think if we allow to move those funds after transfer, we might be violating this?

I mean, as I see it, we should burn that too

maybe we should just burn them, but idk what circles process is for that
I'm more inclined to just burn them, but yeah, I think is worth to ask them

bc of this, i think if we allow to move those funds after transfer, we might be violating this?

Yes, but this is the L1 Adapter, the balance on L2 was already burned, so we still comply with the reqs, buisness wise not sure if owner should be allowed to withdraw or if we just burn

Ah awesome yes, makes sense

0xDiscotech · 2024-07-15T21:26:33Z

src/interfaces/IOpUSDCBridgeAdapter.sol

+ /**
+ * @notice Returns the amount of funds locked that got blacklisted
+ */
+ function blacklistedFunds() external view returns (uint256);


Missing return var and its NATSPEC

0xDiscotech · 2024-07-15T21:33:49Z

src/contracts/L1OpUSDCBridgeAdapter.sol

+ function withdrawBlacklistedFunds() external onlyOwner {
+ uint256 _amount = blacklistedFunds;
+ blacklistedFunds = 0;
+
+ IUSDC(USDC).safeTransfer(owner(), _amount);


Suggested change

function withdrawBlacklistedFunds() external onlyOwner {

uint256 _amount = blacklistedFunds;

blacklistedFunds = 0;

IUSDC(USDC).safeTransfer(owner(), _amount);

function withdrawBlacklistedFunds(address _to) external onlyOwner {

uint256 _amount = blacklistedFunds;

blacklistedFunds = 0;

IUSDC(USDC).safeTransfer(_to, _amount);

0xDiscotech · 2024-07-15T21:35:08Z

@excaliborr Why don't we allow the owner to transfer the blacklisted funds on L2, such as we do on L1 with withdrawBlacklistedFunds()?

0xDiscotech · 2024-07-15T21:36:43Z

I know it doesn't depend from us, but if another minter is added and a mint is done over a blacklisted address it will screw up
totalSupply + blacklistedAmount calculation, right?

excaliborr · 2024-07-16T16:40:15Z

@excaliborr Why don't we allow the owner to transfer the blacklisted funds on L2, such as we do on L1 with withdrawBlacklistedFunds()?

It just felt weird to me, as we would then need to mint them for no reason, if the owner wanted to he could mint himself funds anyway

excaliborr · 2024-07-16T16:41:10Z

I know it doesn't depend from us, but if another minter is added and a mint is done over a blacklisted address it will screw up totalSupply + blacklistedAmount calculation, right?

If another minter is added it shouldn't matter, on L1 we will take the min(balance, burnAmount) anyway

Fixes this: https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/comments#comment-de628a87-0c71-4b62-8f66-52ea92138a50

https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/comments#comment-06da8fcb-e7fd-4993-968b-8f3127d1bd2e

Fixing this: https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/src/contracts/L1OpUSDCBridgeAdapter.sol#L234

# 🤖 Linear Closes OPT-XXX

0xDiscotech · 2024-07-22T22:04:56Z

Wdyt about adding this function in case the blacklisted sender is unblacklisted:

function withdrawBlacklisted() external { 
  if (!isBlacklisted(msg.sender)) {
     USDC.transfer(msg.sender, blacklistedAmount[msg.sender])
    blacklistedAmount -   blacklistedAmount[msg.sender];
    blacklistedAmount[msg.sender] = 0;
    emit ...
  }
}

0xDiscotech

Missing unit tests for the whole catch logic on l1Adapter#receiveMessage.
Missing unit tests for the userBlacklistedFunds update on l2Adapter#receiveMessage

0xDiscotech · 2024-07-23T14:12:05Z

src/contracts/L2OpUSDCBridgeAdapter.sol

+ * @param _user The user to withdraw the funds for
+ */
+ function withdrawBlacklistedFunds(address _user) external override {
+ // TODO: Add explicit check if migration has happend


Missing check on the TODO here, or to be done on a next PR?

next PR, hex is adding this logic in his, intentionally left it out

0xDiscotech · 2024-07-23T14:15:24Z

test/integration/L1OpUSDCBridgeAdapter.t.sol

+
+ _mintSupplyOnL2(optimism, OP_ALIASED_L1_MESSENGER, _blacklistedAmount);
+
+ assertEq(l2Adapter.blacklistedFunds(), _blacklistedAmount);


I know that bef the call blacklistedFunds() is equal to 0, but don't you think adding it before the call and then compare the var is more future proof in case something changes on the setup? Like

uint256 _blacklistedFundsBef = l2Adapter.blacklistedFunds(); // Mint the supply on L2 assertEq(l2Adapter.blacklistedFunds(), _blacklistedFundsBef + _blacklistedAmount); ....

Maybe this can be applied to other tests also on another PR? It might be worth to check IMO

0xDiscotech

Amazing stuff! 🔥

fix: edgecase where user is blacklisted with an inflight message

e98c1a1

hexshire reviewed Jul 15, 2024

View reviewed changes

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

hexshire reviewed Jul 15, 2024

View reviewed changes

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

fix: linter

85c3d95

hexshire reviewed Jul 15, 2024

View reviewed changes

src/contracts/L1OpUSDCBridgeAdapter.sol Show resolved Hide resolved

hexshire reviewed Jul 15, 2024

View reviewed changes

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

0xDiscotech reviewed Jul 15, 2024

View reviewed changes

fix: comments

c2965f2

hexshire approved these changes Jul 16, 2024

View reviewed changes

Merge branch 'audit/spearbit' into fix/blacklist

6225dc3

0xDiscotech approved these changes Jul 16, 2024

View reviewed changes

0xDiscotech and others added 5 commits July 22, 2024 13:58

feat: add burned amount value on migration complete event (#107)

b15d2d2

Fixes this: https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/comments#comment-de628a87-0c71-4b62-8f66-52ea92138a50

fix: add events (#113)

fa2e6f8

https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/comments#comment-06da8fcb-e7fd-4993-968b-8f3127d1bd2e

refactor: user nonces (#108)

5f2d20a

Fixing this: https://cantina.xyz/code/4b344c50-c217-4624-9e08-4298ffb5df47/src/contracts/L1OpUSDCBridgeAdapter.sol#L234

fix: deploy through portal (#115)

f144621

# 🤖 Linear Closes OPT-XXX

fix: burn blacklisted funds at migration

321a464

excaliborr added 2 commits July 23, 2024 08:16

fix: add ability to withdraw incase a user is un-blacklisted

6a3341f

chore: todo

f0de413

excaliborr force-pushed the fix/blacklist branch from 4bc416a to f0de413 Compare July 23, 2024 12:17

excaliborr added 2 commits July 23, 2024 08:20

chore: comment

0285b7a

fix: conflicts

504282d

0xDiscotech reviewed Jul 23, 2024

View reviewed changes

test: improve

d8629bc

0xDiscotech approved these changes Jul 23, 2024

View reviewed changes

fix: lint

5a96277

0xDiscotech approved these changes Jul 23, 2024

View reviewed changes

excaliborr added 2 commits July 23, 2024 12:37

fix: dont burn blacklisted funds

6d28f1f

fix: lint

8b62ac9

hexshire approved these changes Jul 23, 2024

View reviewed changes

0xDiscotech approved these changes Jul 23, 2024

View reviewed changes

excaliborr merged commit 006a27c into audit/spearbit Jul 23, 2024
4 checks passed

excaliborr deleted the fix/blacklist branch July 23, 2024 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: edgecase where user is blacklisted with an inflight message #105

fix: edgecase where user is blacklisted with an inflight message #105

excaliborr commented Jul 15, 2024

0xDiscotech Jul 15, 2024

excaliborr Jul 16, 2024

0xDiscotech Jul 15, 2024

excaliborr Jul 16, 2024

0xDiscotech Jul 16, 2024

0xDiscotech Jul 16, 2024

0xDiscotech Jul 16, 2024

excaliborr Jul 16, 2024 •

edited

Loading

0xDiscotech Jul 16, 2024

0xDiscotech Jul 15, 2024 •

edited

Loading

excaliborr Jul 16, 2024

0xDiscotech Jul 15, 2024

excaliborr Jul 16, 2024

0xDiscotech commented Jul 15, 2024

0xDiscotech commented Jul 15, 2024

excaliborr commented Jul 16, 2024 •

edited

Loading

excaliborr commented Jul 16, 2024

0xDiscotech commented Jul 22, 2024 •

edited

Loading

0xDiscotech left a comment

0xDiscotech Jul 23, 2024

excaliborr Jul 23, 2024

0xDiscotech Jul 23, 2024

0xDiscotech left a comment


		_mintSupplyOnL2(optimism, OP_ALIASED_L1_MESSENGER, _blacklistedAmount);

		assertEq(l2Adapter.blacklistedFunds(), _blacklistedAmount);

fix: edgecase where user is blacklisted with an inflight message #105

fix: edgecase where user is blacklisted with an inflight message #105

Conversation

excaliborr commented Jul 15, 2024

🤖 Linear

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 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDiscotech Jul 15, 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

0xDiscotech commented Jul 15, 2024

0xDiscotech commented Jul 15, 2024

excaliborr commented Jul 16, 2024 • edited Loading

excaliborr commented Jul 16, 2024

0xDiscotech commented Jul 22, 2024 • edited Loading

0xDiscotech left a 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

0xDiscotech left a comment

Choose a reason for hiding this comment

excaliborr Jul 16, 2024 •

edited

Loading

0xDiscotech Jul 15, 2024 •

edited

Loading

excaliborr commented Jul 16, 2024 •

edited

Loading

0xDiscotech commented Jul 22, 2024 •

edited

Loading