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 a bug reported by Quillaudit #14938

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Oct 11, 2023

@adaki2004 Dani, this is a clear bug, but how come our A5 test bridge still works? or does it? Maybe you and @KorbinianK can take a look at some ERC20 bridge txs to check who received the bridged tokens.

@dantaik dantaik marked this pull request as ready for review October 11, 2023 12:44
@dantaik dantaik requested a review from adaki2004 October 11, 2023 12:44
@vercel
Copy link

vercel bot commented Oct 11, 2023

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

Name Status Preview Comments Updated (UTC)
bridge-ui-v2-a5 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2023 0:48am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
bridge-ui-v2-internal ⬜️ Ignored (Inspect) Visit Preview Oct 11, 2023 0:48am

@adaki2004
Copy link
Contributor

@adaki2004 Dani, this is a clear bug, but how come our A5 test bridge still works? or does it? Maybe you and @KorbinianK can take a look at some ERC20 bridge txs to check who received the bridged tokens.

It indeed a bug but not modifying the working behavior by nature.

  1. So when we call on the source chain sendToken it will trigger:
    function _encodeDestinationCall(
        address user,
        address token,
        address to,
        uint256 amount
    )

it basically puts together the receiveToken() parameters to be called with on the destination chain.

  1. The receiveToken() params as follows:
    function receiveToken(
        CanonicalERC20 calldata ctoken,
        address from,
        address to,
        uint256 amount
    )

The user address in this case will be 0, but it does not matter (technically) because we use the from only in the even emitted, so not modifying the working behavior.

Good catch, it shall be indexable and so we need to fill this properly!

@dantaik
Copy link
Contributor Author

dantaik commented Oct 11, 2023

Screenshot 2023-10-11 at 22 00 09

But we use the to address to send tokens/Ether, right? So it matters and user should not have received any tokens after briding.

@adaki2004
Copy link
Contributor

adaki2004 commented Oct 11, 2023

Screenshot 2023-10-11 at 22 00 09

But we use the to address to send tokens/Ether, right? So it matters and user should not have received any tokens after briding.

Yes, but the to address is correct. This (incorrect) represents the from address in this receiveToken() call, which only used in the event emitted.

@dantaik dantaik enabled auto-merge October 11, 2023 14:28
@dantaik dantaik requested a review from davidtaikocha October 12, 2023 02:50
@dantaik dantaik added this pull request to the merge queue Oct 12, 2023
Merged via the queue into main with commit 99b200b Oct 12, 2023
7 of 8 checks passed
@dantaik dantaik deleted the dantaik-patch-1 branch October 12, 2023 05:46
@github-actions github-actions bot mentioned this pull request Oct 12, 2023
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