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(portal-contract): wrong 'from' when depositing from the contract through Portal.onApprove function #275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0x6e616d
Copy link
Member

@0x6e616d 0x6e616d commented Oct 1, 2024

Resolved: External Issue #39

I added a new test for these changes but removed because it only worked with forge test --match-contract OptimismPortal2_Test.

With the contract DeploymentSummaryFaultProofs_TestOptimismPortal that is using Kontrol, vm.startPrank didn't work with l2NativeToken.mint(value) or l2NativeToken.approveAndCall: issue

So we can add the test later. My test

/// @dev Tests that `onApprove` succeeds for an EOA.
    function testFuzz_onApprove_eoa_succeeds(
        address _to,
        uint32 _gasLimit,
        uint256 _value,
        uint256 _mint,
        bool _isCreation,
        bytes memory _data
    )
    external
    {
        _gasLimit = uint32(
            bound(
                _gasLimit,
                optimismPortal2.minimumGasLimit(uint32(_data.length)),
                systemConfig.resourceConfig().maxResourceLimit
            )
        );
        if (_isCreation) _to = address(0);

        vm.startPrank(depositor, depositor);
        l2NativeToken.faucet(_value);

        bytes memory onApproveData = abi.encodePacked(_to, _mint, _gasLimit, _data);

        // EOA emulation
        vm.expectEmit(true, true, true, true);
        emit TransactionDeposited(depositor, _to, 0, onApproveData);

        l2NativeToken.approveAndCall(address(optimismPortal2), _value, onApproveData);
    }

    /// @dev Tests that `onApprove` succeeds for a contract.
    function testFuzz_onApprove_contract_succeeds(
        address _to,
        uint32 _gasLimit,
        uint256 _value,
        uint256 _mint,
        bool _isCreation,
        bytes memory _data
    )
    external
    {
        _gasLimit = uint32(
            bound(
                _gasLimit,
                optimismPortal2.minimumGasLimit(uint32(_data.length)),
                systemConfig.resourceConfig().maxResourceLimit
            )
        );
        if (_isCreation) _to = address(0);

        vm.startPrank(address(this));
        l2NativeToken.faucet(_value);

        bytes memory onApproveData = abi.encodePacked(_to, _mint, _gasLimit, _data);

        vm.expectEmit(true, true, true, false);
        emit TransactionDeposited(AddressAliasHelper.applyL1ToL2Alias(address(this)), _to, 0, hex"");

        l2NativeToken.approveAndCall(address(optimismPortal2), _value, onApproveData);
    }

@0x6e616d 0x6e616d changed the title DRAFT: fix(portal-contract): wrong 'from' when depositing from the contract through Portal.onApprove function fix(portal-contract): wrong 'from' when depositing from the contract through Portal.onApprove function Oct 15, 2024
Copy link
Member

@rlgns98kr rlgns98kr left a comment

Choose a reason for hiding this comment

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

Thank you very much, Nam!
I think we should not merge this yet, shouldn't we?

@0x6e616d
Copy link
Member Author

0x6e616d commented Oct 15, 2024

Thank you very much, Nam! I think we should not merge this yet, shouldn't we?

I think we can submit PR to the external audit team and we'll merge after we finalize the audit.

@theo-learner
Copy link
Collaborator

theo-learner commented Oct 15, 2024

@tokamak-network/layer2-core I will submit this PR to Sherlock. Do we need an additional review of this code? If you need it, please let me know by tomorrow.

@nguyenzung
Copy link
Member

I think the code needs to be updated base on the commit c3a2bd6

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.

obront - L1 contract can evade aliasing, spoofing unowned L2 address
4 participants