Align data location of interface with implementation

[cameel]

The reason behind the PR has already been explained well by @chriseth in a similar PR to OpenZeppelin (OpenZeppelin/openzeppelin-contracts#3293) so I'll copy that description here:

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 { }

In case of Gnosis, the function in question is isValidSignature from ISignatureValidator.

[rmeissner]

Totally makes sense to merge this, but I was wondering if this bug is triggered by the Safe code. We do have tests that call this code and it works as expected. (This is important to understand if we have to redeploy this contract and migrate existing Safes).

[rmeissner]

Note: it seems the repo is not properly setup for external migrations, I will look into this

[cameel]

No, I don't think your code is affected (other than you'd get a compilation error once we release a fix). The risk of calldata being reinterpreted as memory exists only for internal calls. You seem to be calling isValidSignature() only externally and in external calls both calldata and memory arrays are encoded the same way.

[cameel]

Any chance this will be merged soon?

[rmeissner]

We are currently in the progress of migrating repositories and organisations. And as this is not security critical we didn't give it any priority. Are there any external dependencies that are related to this?

[cameel]

Ok, thanks for the info. It's not critical for us either, it's just pretty trivial and would let us have one less workaround in the testing scripts.

By dependencies, do you mean other PRs that would have to be merged in sync in the compiler or other projects? Not really. It's simply that the form you have now will be disallowed. The form this PR changes it to worked before and will still work.

[rmeissner]

With dependencies I meant if there is a project that is depending on this PR being merged ;)

I will try to get this done as soon we are are done with the coordination of the org migration.

[cameel]

Thanks!

No, I don't think there are any dependencies like that. For us it's just that Gnosis Safe is one of the projects we use to test the compiler binary in CI. We compile it and run its tests on every PR. We can add a workaround, it's just always easier if the problem gets fixed upstream :)

[rmeissner]

Could you rebase this PR too, then the test will run :)

[cameel]

Sure. Both PRs rebased now.

[mmv08]

@rmeissner Can we merge this? This is relevant for our 1.4.0 release #440