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

Change GMP precompile to rely only on amount transferred #2244

Merged
merged 2 commits into from Apr 20, 2023

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Apr 18, 2023

What does it do?

Internally, Wormhole does some "normalization" of asset amounts, relying on the wrapper ERC20's decimals() to modify the amount specified in the payload. The following fn is internally called by the bridge where decimals is taken from the ERC20:

    function deNormalizeAmount(
        uint256 amount,
        uint8 decimals
    ) internal pure returns (uint256) {
        if (decimals > 8) {
            amount *= 10 ** (decimals - 8);
        }
        return amount;
    }

The fix is quite simple: just detect the amount transferred (this was already done) and ignore the amount in the payload (previously we took the min() of the two).

While the fix is simple, I'd like a better understanding of the design behind the normalization technique, and I'd also like a way to catch this in tests.

@notlesh notlesh added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes labels Apr 18, 2023
@nbaztec
Copy link
Contributor

nbaztec commented Apr 19, 2023

I agree, a test would be great to have here, as an important change is in consideration.

@crystalin crystalin mentioned this pull request Apr 19, 2023
15 tasks
@librelois librelois merged commit 5d1317a into master Apr 20, 2023
22 checks passed
@librelois librelois deleted the notlesh-gmp-rely-on-amt-transferred branch April 20, 2023 12:02
librelois pushed a commit that referenced this pull request Apr 20, 2023
Co-authored-by: crystalin <alan.sapede@gmail.com>
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
@crystalin crystalin changed the title GMP: Rely only on amount transferred Change GMP precompile to rely only on amount transferred Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants