`returndatacopy(0, 0, returndatasize())` violates `memory-safe`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x6e114370ead425fb156a1809077c414cadb3efd8071281760fd3d0ff0b5d60eb Severity: medium

Description: Description

returndatacopy(0, 0, returndatasize()) violates memory-safe. if data returned from the external delegate-call doesn't fit into scratch space.

Attack Scenario\ Describe how the vulnerability can be exploited.

1. Proof of Concept (PoC) File

    fallback() external payable {
        address singleton = _SINGLETON;
        uint256 x = _X;
        uint256 y = _Y;
        P256.Verifiers verifiers = _VERIFIERS;

        // solhint-disable-next-line no-inline-assembly
        assembly {
            // Forward the call to the singleton implementation. We append the configuration to the
            // calldata instead of having the singleton implementation read it from storage. This is
            // both more gas efficient and required for ERC-4337 compatibility. Note that we append
            // the configuration fields in reverse order since the fields are packed, and this makes
            // it so we don't need to mask any bits from the `verifiers` value. This computes `data`
            // to be `abi.encodePacked(msg.data, x, y, verifiers)`.
            let data := mload(0x40)
            mstore(add(data, add(calldatasize(), 0x36)), verifiers)
            mstore(add(data, add(calldatasize(), 0x20)), y)
            mstore(add(data, calldatasize()), x)
            calldatacopy(data, 0x00, calldatasize())

            let success := delegatecall(gas(), singleton, data, add(calldatasize(), 0x56), 0, 0)
            returndatacopy(0, 0, returndatasize())
            if iszero(success) {
                revert(0, returndatasize())
            }
            return(0, returndatasize())
        }
    }

https://github.com/safe-global/safe-modules/blob/9a18245f546bf2a8ed9bdc2b04aae44f949ec7a0/modules/passkey/contracts/SafeWebAuthnSignerProxy.sol#L73

It won't violate the memory requirements as long as the returndata fits into the memory's scratch space otherwise it will create issues.

Modify it to make memory-safe

[0xEricTee]

Hi there, thanks for the submission. I believe returndatasize() will not exceed the 64 byte scratch space because singleton address is trusted and known contract.

[Tri-pathi]

I believe returndatasize() will not exceed the 64 byte scratch space

@nlordell what is your pov ?

[nlordell]

I agree that this assembly violates memory safety (there is a getConfiguration function that returns more than 64 bytes). However, the assembly block is explicitly not tagged as memory safe, and always returns after the block, so this would not be an issue. Also, since we use the bytecode optimizer when compiling, having non-memory-safe assembly should not affect the compiler's ability to optimize code.

[Tri-pathi]

@nlordell

Throughout the codebase, most of the assembly blocks are memory-safe, I believe they were implemented this way for a purpose.

I explored more but didn't find much information about issues due to memory-unsafe blocks and their effect in the YUL/IR optimizer since how IR or Any optimizer works under the hood is very superficial. Though the optimizer might change the order of opcodes for optimization.

Also memory-safe is crucial in overall safe ecosystem. eg : discussion. Above assembly block can be made memory-safe to avoid potential issues due to optimizer and other memory related issue.

Hence I think issue should be valid and Acknowledged

PS: Please attach some reference to research more about memory-safe effect and issues

[nlordell]

Memory safe assembly allows the IR optimizer to make additional optimizations. Memory unsafe assembly prevents the IR optimizer from making those additional optimizations.

Since this is "gas optimization" related, it is not in scope for this audit competition.

[nlordell]

Here is some more information on memory-safe and optimizations: https://github.com/safe-global/safe-smart-account/issues/544