5afe / safe-core-protocol-specs

Safe{Core} Protocol is an open, modular framework to make smart accounts secure, portable, and composable.
GNU General Public License v3.0
66 stars 13 forks source link

Memory-Safe Proxy Code Without Allocation #67

Closed nlordell closed 10 months ago

nlordell commented 11 months ago

This PR modifies the account fallback code to no longer use allocations, as they are not required for memory-safety. From the docs:

In particular, a memory-safe assembly block may only access the following memory ranges:

  • [...]
  • Temporary memory that is located after the value of the free memory pointer at the beginning of the assembly block, i.e. memory that is “allocated” at the free memory pointer without updating the free memory pointer.

[...]

On the other hand, the following code is memory safe, because memory beyond the location pointed to by the free memory pointer can safely be used as temporary scratch space:

assembly ("memory-safe") {
  let p := mload(0x40)
  returndatacopy(p, 0, returndatasize())
  revert(p, returndatasize())
}

I also did some investigation and found that when variables are moved into memory, the space gets reserved before user code starts, meaning that the free memory pointer already accounts for the reserved space (i.e. setting variables cannot write past the free memory pointer ever). With that in mind, we do not need to update the free memory pointer when we write past it for scratch space.

cc @mmv08

mmv08 commented 10 months ago

I'm struggling to understand how it works. It says that the memory beyond the location can be safely used, but how? If we put some data to the memory location P, now the safe space is P + data size (in my understanding), but it didn't get updated. Or does the returndatacopy opcode update it under the hood?

mmv08 commented 10 months ago

Is that related to the memoryguard(…) you mentioned in a comment in the original issue?

nlordell commented 10 months ago

Is that related to the memoryguard(…) you mentioned in a comment in the original issue?

Exactly!

Typically, the Solidity compiler sets the free memory pointer to 0x80 on startup (this is the end of the “reserved” memory regions where new allocations can be made). However, when it decides to move variables into memory, it reserves the extra needed space right on startup. Specifically, the initial value of the free memory pointer will instead be 0x80 + requiredReservedSpace. In the example linked in the issue, the very first thing that will be executed by the contract is:

                let _1 := memoryguard(0x0440)
                mstore(64, _1)

This translates to the following opcodes:

[...]
PUSH2 0x440
[DUP1] # compiler optimization as the 0x440 gets used for a CALLDATACOPY right after
PUSH1 0x40
MSTORE
[...]

So, basically the free memory pointer is initialized to something large enough so that the required space for the memory variables is already allocated.

mmv08 commented 10 months ago

Is that related to the memoryguard(…) you mentioned in a comment in the original issue?

Exactly!

Typically, the Solidity compiler sets the free memory pointer to 0x80 on startup (this is the end of the “reserved” memory regions where new allocations can be made). However, when it decides to move variables into memory, it reserves the extra needed space right on startup. Specifically, the initial value of the free memory pointer will instead be 0x80 + requiredReservedSpace. In the example linked in the issue, the very first thing that will be executed by the contract is:

                let _1 := memoryguard(0x0440)
                mstore(64, _1)

This translates to the following opcodes:

[...]
PUSH2 0x440
[DUP1] # compiler optimization as the 0x440 gets used for a CALLDATACOPY right after
PUSH1 0x40
MSTORE
[...]

So, basically the free memory pointer is initialized to something large enough so that the required space for the memory variables is already allocated.

Understood now! Thanks so much for the detailed explanation

nlordell commented 10 months ago

@mmv08 - do you think we should merge as is or is further clarification needed?

mmv08 commented 10 months ago

@mmv08 - do you think we should merge as is or is further clarification needed?

Ah, sorry, I forgot to approve. It can be merged for sure