Solidity: Passing calldata to memory modifier

[emilbayes]

Description

I have a modifier hasQuorum(bytes[] memory signatures, bytes memory data) that I apply to my external contract functions, eg. function addOwner(bytes[] calldata signatures, address owner) external hasQuorum(signatures, abi.encodePacked(owner)) with a check on the length of the signatures array. However the array seems to be length zero always in the modifier.

I tried making a small test case to figure out why, but this causes an internal compiler error.

Environment

• Compiler version: 0.5.12 (but also tested with 0.5.14-nightly.2019.12.9)
• Target EVM version (as per compiler settings): Compiler default
• Framework/IDE (e.g. Truffle or Remix): Both
• EVM execution environment / backend / blockchain client: N/A
• Operating system: macOS and Remix

Steps to Reproduce

The following contract fails with "InternalCompileError":

pragma experimental ABIEncoderV2;

contract ModifierCalldata {
    event Counter(uint count);

    modifier countMod (bytes[] memory data) {
        emit Counter(data.length);
        _;
    }

    function count (bytes[] calldata data)
        external
        countMod(data)
        returns(uint)
    {
        return data.length;
    }
}

[chriseth]

Can please you also provide the code that compiles and causes the length to be zero?

[emilbayes]

@chriseth I haven't been able to reduce to a minimal example yet, but it's this function on a larger contract: https://github.com/hyperdivision/eth-multisig-wallet/blob/bfcdcef306f5393f28875ccdf22c06029defc972/contracts/Wallet.sol#L30

Here's the line that evaluates to false despite owners.length == 1 and signatures.length == 1: https://github.com/hyperdivision/eth-multisig-wallet/blob/bfcdcef306f5393f28875ccdf22c06029defc972/contracts/Quorum.sol#L22

Before the functions in Wallet where public instead of external and calldata was memory, which all worked.

[emilbayes]

Here's another faulty contract causing: InternalCompilerError: I sense a disturbance in the stack: 3 vs 4

pragma experimental ABIEncoderV2;

contract ModifierCalldata {
    event Counter(uint count);

    function countMod (bytes[] memory data) internal {
        emit Counter(data.length);
    }

    function count (bytes[] calldata data)
        external
        returns(uint)
    {
        countMod(data);
        return data.length;
    }
}

[emilbayes]

I guess it makes sense that this type of "casting" can't just happen like that (or would require copying the calldata to memory), but modifiers and internal functions are not allowed to access calldata any other way?

[Marenz]

Let's fix those ICE's first and maybe you can create a minimal test case then

[Marenz]

So, the problem seems to be that we simply didn't implement the code for this. I am not very familiar with the calldata/memory layout at this point so I leave it to someone who knows this better.

A starting point for whoever picks this up is https://github.com/ethereum/solidity/blob/develop/libsolidity/codegen/CompilerUtils.cpp#L940-L898
which is missing the appropriate code.

[ekpyron]

A lot of cases related to calldata arrays were left unimplemented intentionally, when they were introduced, with the idea of them only being properly implemented in ABIEncoderV2 (especially since you can always change things to "public" and "memory" as before).
And there should be either proper errors or internal compiler errors for all these cases - but if there is indeed a case like described in this issue, with wrong behaviour and no error at all, then that's a bug.

[emilbayes]

@ekpyron But the semantics of how calldata is represented at the EVM level are very different to memory, right?

[ekpyron]

Ok, these are all ABIEncoderV2 cases - for those all of this should indeed be implemented eventually.

[ekpyron]

@emilbayes Yes, and passing around calldata arrays is way cheaper, that's why they were introduced - to avoid unnecessary copying to memory (before calldata arrays were introduced, on the EVM level, stuff "started" in calldata anyways, but we always copied to memory, which isn't always necessary). But not all of that is implemented yet. I'll look into it for this case here in any case.

[ekpyron]

FYI: independently of the potential bug here (i.e. no compiler error and wrong values, which is quite severe), there's two things that will change in the future:

"casting" calldata to memory will at some point work in all cases (but yes, this indeed involves a full deep copy - at some point that fact will also be explicit in syntax) and it at some point it will be possible to pass calldata arrays around everywhere (including to internal functions).

[emilbayes]

The latter would be nice. Concretely in my use-case I just need to read and "slice" calldata

[ekpyron]

"Fixed" by https://github.com/ethereum/solidity/issues/7635 (resp. "won't fix" for the old code generator). Please reopen if there is still a case in which this doesn't cause an "UnimplementedFeatureError" now or if there is a reproducible case (even for older compilers) in which this actually causes invalid values and not an internal compiler error.