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

Align data location of interface with implementation. #3293

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Mar 24, 2022

First, I would like to apologize. Somehow the issue ethereum/solidity#10900 got lost in our backlog. We are currently fixing it and discovered one instance of this in openzeppelin. This PR fixes it.

More details:

The function hashProposal in the interface is declared with calldata parameters while its implementation in Governor uses memory. This leads to invalid code being generated whenever someone calls the hashProposal function internally on the interface instead of the implementation. This can only happen if you have an (abstract) contract that inherits from the interface, but not from the implementation, but is still part of an inheritance hierarchy that also has the implementation.

Simplified example:

abstract contract I {
        function f(uint[] calldata x)  virtual internal;
}
contract C is I {
        uint public data;
        // The override checker in the compiler does not complain
        // here - this is the bug in the compiler.
        function f(uint[] memory x)  override internal {
                data = x[0];
        }
}
abstract contract D is I {
        function g(uint[] calldata x)  external {
                // Since D only "knows" `I`, the signature of `f` uses calldata,
                // while the virtual lookup ends up with `C.f`, which uses memory.
                // This results in the calldata pointer `x` being passed and interpreted
                // as a memory pointer.
                f(x);
        }
}
contract X is C, D { }

@Amxx
Copy link
Collaborator

Amxx commented Mar 24, 2022

Thank you @chriseth for finding that !

I guess we will want to add a notification in the Changelog (afaik, this is not a breaking change)

@frangio
Copy link
Contributor

frangio commented Mar 28, 2022

Merged in #3295 with updated changelog.

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