Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

low level staticcall return value is not checked #14

Open
hats-bug-reporter bot opened this issue Jun 19, 2024 · 6 comments · Fixed by safe-global/safe-modules#457 · May be fixed by #22
Open

low level staticcall return value is not checked #14

hats-bug-reporter bot opened this issue Jun 19, 2024 · 6 comments · Fixed by safe-global/safe-modules#457 · May be fixed by #22
Labels
bug Something isn't working Lead - low low

Comments

@hats-bug-reporter
Copy link

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.

@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Jun 19, 2024
@nlordell
Copy link
Collaborator

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:

image

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

@nlordell
Copy link
Collaborator

nlordell commented Jun 20, 2024

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 nlordell added question Further information is requested invalid This doesn't seem right low and removed question Further information is requested invalid This doesn't seem right labels Jun 20, 2024
@nlordell
Copy link
Collaborator

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 nlordell linked a pull request Jun 21, 2024 that will close this issue
@nlordell
Copy link
Collaborator

All in all, very nice find :)

Here is a PR that should address the issue: #22

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

@0xRizwan
Copy link

Fix looks good.

nlordell added a commit to safe-global/safe-modules that referenced this issue Jul 5, 2024
Fixes
hats-finance#14,
also see
hats-finance#22
for some additional context.

This PR changes the `_sha256` implementation to check the result from
the static call. There is a very subtle bug with not checking, where,
for very large inputs, you would be able to get the precompile to revert
but have the function finish executing successfully (and use whatever is
in the scratch space as the digest).

Note that **we do not check the length of the `returndata`**. This is
intentional and the same thing that the Solidity compiler does for the
builtin `sha256` function.
@fonstack fonstack reopened this Jul 9, 2024
@nlordell
Copy link
Collaborator

nlordell commented Jul 9, 2024

Oops, looks like GitHub automatically closed the issue - sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Lead - low low
Projects
None yet
4 participants