Disallow ``codecopy`` in pure functions (and check for other cases)

ethereum / solidity

Solidity, the Smart Contract Programming Language

https://soliditylang.org

GNU General Public License v3.0

23.29k stars 5.76k forks source link

Disallow ``codecopy`` in pure functions (and check for other cases) #12260

Open ekpyron opened 2 years ago

ekpyron commented 2 years ago

Related to https://github.com/ethereum/solidity/issues/8153 and following https://github.com/ethereum/solidity/pull/12256 we should make sure we actually are as strict as we want to be for pure functions.

In particular, we should disallow codecopy with 0.9.0, but we should double-check if there is other cases that we should strengthen as well.

chriseth commented 2 years ago

Isn't disallowing access to msg.data also part of this issue?

ekpyron commented 2 years ago

I mean, I can also just do it right away, maybe that's safest for not missing it again.

ekpyron commented 2 years ago

Isn't disallowing access to msg.data also part of this issue?

If we agree that we should, then yes, absolutely :-)! I'm all for that, but I wasn't sure we had consensus about that.

ekpyron commented 2 years ago

Hm... I'm just looking through the instruction list... and hit CALLDATALOAD :-)... That one we can't really disallow I guess... but if we allow it, we might as well keep msg.data...

The problem is still that "externally pure" is different from "internally pure". An external function call I can compile-time evaluate, even if it involves msg.data and calldataload - but an internal one I cannot...

axic commented 2 years ago

Well basically external pure can have different rules, than private/public/internal. We had another issue for tracking the memory-mutability of pure, that should be also revived.

ekpyron commented 2 years ago

Hm, yeah... I actually thought we couldn't just use different rules due to public being both internal and external, but it actually makes perfect sense to apply the stricter internal rules to public...

Anyways, I pushed #12261 for the obvious cases I saw that shouldn't be pure, i.e. codecopy and codesize - the rest we can do, once we decide if to split external and internal analysis or what else to do.

chriseth commented 2 years ago

What about internal functions that have calldata parameters? Can they use inline assembly and thus calldataload to access their parameters?

ekpyron commented 2 years ago

Yeah - which is a problem. If we allow loading something from calldata, we effectively allow loading anything from calldata and thus might as well keep msg.data pure... Then again, I'm not sure if we should ever even try to actually compile-time-evaluate inline assembly anyways...

So maybe restricting pure to the notion of "external pure" only is enough (i.e. disallow accessing code, but generally allowing to access calldata however one likes, including via msg.data) - and whether an internal pure function can be compile-time evaluated we can have the compiler decide without a special syntactic marker... but I'm not sure...

cameel commented 2 years ago

but we should double-check if there is other cases that we should strengthen as well.

I went through the list of documented opcodes while testing #12861. The only opcodes that seem relevant to this issue are:

returndatasize() (disallowed after #12861)
returndatacopy() (disallowed after #12861)
calldataload() (discussed above)
calldatasize() (discussed above)
calldatacopy() (discussed above)
codesize() (disallowed after #12261)
codecopy() (disallowed after #12261)

So I think we're done here. Can we close this issue now?