low level staticcall return value is not checked

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xc5aa01ec53728cc08191a588e870f0ea5b676be73914963105ecd67ce11c26a7 Severity: low

Description: Description\

    function _sha256(bytes memory input) private view returns (bytes32 digest) {

          . . . .some code

@>            pop(staticcall(gas(), 0x0002, add(input, 0x20), mload(input), 0, 32))
            digest := mload(0)
        }
    }

_sha256() is used to compute the SHA-256 hash of the input bytes. The return value of a low level staticcall is not checked which can be seen in above function. Execution will resume even if the function throws an exception. If the call fails accidentally, this may cause unexpected behaviour in the subsequent program logic.

Low level calls like delegatecall, staticcall and call return values are always checked so that it would revert and function should not assume, it behaved correctly. A revert in case of function failure should be required in this case.

Recommendations\ Explicitely check the return value of staticcall.

[nlordell]

This call is to address 0x0002 which is the SHA-256 precompile. The rationale for not checking this is documented in the code:

            // The SHA-256 precompile is at address 0x0002. Note that we don't check the whether or
            // not the precompile reverted or if the return data size is 32 bytes, which is a
            // reasonable assumption for the precompile, as it is specified to always return the
            // SHA-256 of its input bytes.

Do you think that this assumption is invalid in some way? Note that the precompile is specified in the Ethereum yellow paper to always return successfully and with a returndatasize of 32:

So this would only be an issue for non-EVM compatible chains (which we don't generally support).

[nlordell]

For curiosity, I looked at what the Solidity compiler generates for this code:

    function doSha256(bytes memory data) external pure returns (bytes32 value) {
        value = sha256(data);
    }

And it does this:

                let _5 := staticcall(gas(), 2 , _3, sub(_4, _3), 0, 32)
                if iszero(_5) { revert_forward_1() }
                let expr_11 := shift_left_0(mload(0))
                /// @src 0:322:342  "value = sha256(data)"
                var_value_6 := expr_11
                let expr_12 := expr_11

So, while it checks for non-success - it also does not check that the returnsize is 32. I think the main risk is that the precompile is not supported, in which case it would behave identically to how the sha256 Solidity primitive would behave.

~With this in mind, I would be inclined to mark this as invalid.~

Edit: After more internal discussion, it turns out that my above statement isn't strictly true. It would behave differently for large inputs when the input gas is not enough. As such, I do agree that this is a "low" find.

[nlordell]

We have decided to implement this in the same way that the Solidity compiler emits code:

• Check that the staticcall doesn't revert
• Do not check the return data size.

[nlordell]

All in all, very nice find :)

Here is a PR that should address the issue: https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/pull/22

Please let me know if you agree, or have any further concerns with the proposed solution.

[0xRizwan]

Fix looks good.