Incorrect reading of enum variable when sharing storage slot about solidity HOT 5 CLOSED

It's not a bug, The behavior you're showing is correct. I think your expectations about how delegatecall works are incorrect.

When you delegatecall ContractB.executeSomething() it will write to the location of its curType variable, which is slot 0. In a delegate call its storage is simply mapped to ContractA's storage space (i.e. instead of reading/writing ContractB's slot N, you read/write ContractB's slot N). ContractB's bytecode is executed on that remapped storage space. Variable names do not matter.

That's because when ContractB is compiled, all its bytecode knows is that it's supposed to write to a variable located in slot 0. The knowledge of what the variable was called is not preserved and it is not in any way remapped to match the location of a similarly named variable in ContractA. It just writes to slot 0, which, in ContractA's storage space, happens to be variable holding the owner address.

from solidity.

@cameel could you please verify this one more time? I am not saying that the writing of the variable is incorrect, I am saying that the READING of the variable is incorrect (at Contract A, not Contract B) . Actually if you see the the contents of the slot 0 in the hardhat-tracer trace I posted, the variable (at slot 0) is holding the correct value (value = 2) , but when the output to the client is done, the value sent is 0.

It just writes to slot 0, which, in ContractA's storage space, happens to be variable holding the owner address.

all writes are correct. I am saying that the "reading" is incorrect, this line here from the log I posted above:
[SLOAD] 0x0000000000000000000000000000000000000000000000000000000000000000 → 0x000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb92202

means the ContractA have read the slot 0 , and there is the value 02 there. If it gets value = 02, why does it returns 0 instead of 2?
@cameel

from solidity.

Well, I still think that the behavior is correct, but looks like I misinterpreted why you're expecting to see 2 there. You do correctly account for the behavior of delegatecall, but I think you're missing a small detail of how variables are laid out.

As stated in Layout of State Variables in Storage, the curType variable gets packed into a single slot with the inherited owner variable as you correctly observed. But the other relevant point is:

• The first item in a storage slot is stored lower-order aligned.

I.e. variables in a storage slot are placed from right to left and the slot is padded on the left. Which means that in the layouts seen by ContractA and ContractB curType actually does end up in the same slot but at different offsets. Below is an illustration of how both contracts see the storage at the point when the value is being returned.

ContractA's layout:

        ----------------------TTOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO
slot 0  000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb92202
        ------------------------EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE
slot 1  000000000000000000000000e7f1725e7734ce288f8367e1bb143e90bb3f0512

ContractB's layout:

        ----------------------EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEETT
slot 0  000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb92202
        ----------------------------------------------------------------
slot 1  000000000000000000000000e7f1725e7734ce288f8367e1bb143e90bb3f0512

In the above the OO bytes belong to owner, TT to curType and EE to executor. -- bytes are unallocated space.

If you compile the contracts and look at the IR output (specifically --ir-optimized --optimize, unoptimized is more verbose, but still equivalent), you'll see that ContractA is not reading the rightmost byte and somehow returning it wrong. It's reading a byte that's to the left of owner and really has a value of 0:

let value_2 := and(shr(160, sload(0)), 0xff)

Let me know if you still think there's something wonky happening, but after looking at it, I'm still pretty confident that the generated code is behaving correctly.

from solidity.

@cameel yes, I concur (after checking everything) that this problem is due to storage addressing rather than a bug in solidity. When outputting the storage layout with --storage-layout option I got these storage locations:

              {
                "astId": 190, 
                "contract": "contracts/ContractA.sol:ContractA",
                "label": "curType",
                "offset": 20,
                "slot": "0", 
                "type": "t_enum(MyType)272"
              },

and for ContractB:

              {
                "astId": 252,
                "contract": "contracts/ContractB.sol:ContractB",
                "label": "curType",
                "offset": 0,
                "slot": "0",
                "type": "t_enum(MyType)272"
              },

the slots are the same but offsets are different. Thank you very much for the help.

Maybe in future versions of Solidity we could add a warning like "you are using the same label of the same type within different contract files but these variables are not the same in the physical storage" , or something like that. (when delegateCall is made).

from solidity.

Maybe in future versions of Solidity we could add a warning like "you are using the same label of the same type within different contract files but these variables are not the same in the physical storage" , or something like that. (when delegateCall is made).

I don't think that's ever going to happen with the current syntax. There are very few ways in which names in the language actually matter (basically just selectors). Automatically making this kind of implicit assumption for any delegatecall goes hard against the direction in which the language is heading.

The only way I see something like that making sense is if we introduced some syntax to make that expectation explicit. E.g. by somehow marking a variable declaration in two contracts as occupying the same location. Actually, this sounds related to the often requested feature of explicitly specifying storage locations (#597). If we had that, you could just set the same location for both variables. And, if we allowed using not just absolute location but also relative to some other variable (potentially from another contract), it would cover this use case. I'm not sure this is a good idea, but technically, this could work. You could try to propose it in that issue and see what the general sentiment about that is. I suspect it would be a pretty niche thing though.

from solidity.