brinktrade / brink-core

Core smart contracts for Brink accounts
GNU General Public License v3.0
12 stars 7 forks source link

Align data location of interface with implementation #52

Open cameel opened 2 years ago

cameel commented 2 years ago

The reason behind the PR has already been explained well by @chriseth in a similar PR to OpenZeppelin (https://github.com/OpenZeppelin/openzeppelin-contracts/pull/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 Brink, the function in question is isValidSignature from ISignatureValidator.