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(protocol): fix recall not working with bridged tokens #15679

Merged
merged 6 commits into from
Feb 10, 2024

Conversation

Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented Feb 8, 2024

The token passed into receiveToken is always the canonical token, but the recall code checks if bridgedToCanonical[ctoken.addr].addr exists to know if it's a bridged or canonical token, which seems incorrect because it's never a bridged token that is passed in. So the current code does not support recalling on bridged token contracts correctly because all tokens are always seen as canonical.

Previous code:

if (bridgedToCanonical[ctoken.addr].addr != address(0)) {
    IBridgedERC20(ctoken.addr).mint(message.owner, amount);
} else {
    ERC20(ctoken.addr).safeTransfer(message.owner, amount);
}

The first check would always be false because ctoken.addr is always a canonical token and so the bridged path would never be taken. So if indeed the bridged token needs to be used on the chain, it would not work because it would also go through the canonical token path.

I believe the code to transfer the tokens in onMessageRecalled should be the same code that is used in receiveToken with the same token address deduction to know if the bridged variant needs to be used or the canonical one.

There also seems to be no test that tests the ERC20 recall, so I'm trying to add one but it's not working yet because I'm no familiar with the test code. But working on it!

@Brechtpd Brechtpd requested a review from adaki2004 February 8, 2024 00:38
Copy link
Contributor

@adaki2004 adaki2004 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Brechtpd Brechtpd changed the title fix(protocol): fix recall token deduction (maybe) fix(protocol): fix recall not working with bridged tokens Feb 8, 2024
Copy link

vercel bot commented Feb 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
bridge-ui-v2-a6 ✅ Ready (Inspect) Visit Preview Feb 8, 2024 3:02pm

@Brechtpd Brechtpd enabled auto-merge (squash) February 10, 2024 15:21
Co-authored-by: xiaodino <[email protected]>
Co-authored-by: jeff <[email protected]>
Co-authored-by: d1onys1us <[email protected]>
Co-authored-by: D <[email protected]>
Co-authored-by: Keszey Dániel <[email protected]>
Co-authored-by: Daniel Wang <[email protected]>
@Brechtpd Brechtpd disabled auto-merge February 10, 2024 17:02
# Conflicts:
#	packages/protocol/test/tokenvault/ERC20Vault.t.sol
@Brechtpd Brechtpd merged commit dd2c33d into main Feb 10, 2024
14 checks passed
@Brechtpd Brechtpd deleted the alpha-6-recall-fix branch February 10, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants