Mismatching data locations are allowed in overrides

[nventuro]

It is possible to override functions that expect memory arguments with implementations that expect them in calldata. Offending code compiles with no warnings, but causes runtime errors.

I've observed this behavior in both 0.7 and 0.8 versions, but haven't tested how far back it goes.

Sample contract:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

pragma experimental ABIEncoderV2;

abstract contract Base {
    struct Foo {
        uint256 foo;
    }
    
    function bar(Foo memory f) public pure returns (uint256) {
        return baz(f);
    }
    
    function baz(Foo memory f) internal virtual pure returns (uint256);
}

contract FaultyDerived is Base {
    function baz(Foo calldata f) internal override pure returns (uint256) {
        return f.foo;       
    }
}

contract CorrectDerived is Base {
    function baz(Foo memory f) internal override pure returns (uint256) {
        return f.foo;       
    }
}

I haven't tested too much the runtime issues, but I have seen some inconsistent behavior. When trying the example above on Remix, CorrectDerived works correctly as expected, but FaultyDerived returns 0 always. This is also true if foo is an address instead of a uint.

However, on my local Hardhat environment in the project where I found this, I get reverts when attempting to access fields in the offending struct. I imagine this is related to either the fact that my structs are more complex, or that my project has already performed a few memory allocations at the moment the issue is triggered.

[frangio]

I haven't been able to debug this on Remix because it hangs, but I imagine this is reinterpreting a memory pointer as a calldata pointer, or vice versa.

This looks like a pretty serious bug that could be used to write underhanded code.

[cameel]

The ability to use a different data location in the overriding function seems intentional (we even have syntax tests covering it: calldata_memory_interface_struct.sol, calldata_memory.sol, calldata_memory_interface_struct.sol). This is not mentioned in Contracts > Function Overriding though and I think it should be. The example you gave compiles as far back as 0.6.9 which I think is the version that introduced calldata parameters.

The fact that it reverts or returns zero does look like a bug though. We should investigate and also add a semantic test for that.

[chriseth]

This was neccessary when external functions were still restricted to calldata and public to memory. Otherwise it was impossible to override an external function with a public one. What should not be allowed is to override memory with calldata. If you do this, calling the function locally can lead to problems. In both cases, we should check that the parameter type is properly determined.

[azige]

The worst thing is such codes can be compiled by solc 0.8.12+commit.f00d7308.Emscripten.clang without any warnings and errors. I was trapped once by this. I think the compiler should raise a warning until a proper solution for this issue is made.