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

[False-Positive]:msg.value in a loop when msg.value is not transferred #2237

Closed
CJ42 opened this issue Nov 17, 2023 · 1 comment
Closed

[False-Positive]:msg.value in a loop when msg.value is not transferred #2237

CJ42 opened this issue Nov 17, 2023 · 1 comment

Comments

@CJ42
Copy link
Contributor

CJ42 commented Nov 17, 2023

Describe the issue:

In the following code snippet, Slither detects the following:

image

This seems invalid as msg.value here is just checked as a conditional to ensure no value is sent to this function. In any case, no logic inside this function (including inside the internal _transfer function) forwards msg.value.

Code example to reproduce the issue:

    modifier noNativeTokens() {
        require(msg.value == 0, "Not aimed to receive native tokens");
        _;
    }
    
   function executeRelayCallBatch(
        bytes[] calldata signatures,
        uint256[] calldata nonces,
        uint256[] calldata validityTimestamps,
        uint256[] calldata values,
        bytes[] calldata payloads
    ) public payable noNativeTokens returns (bytes[] memory) {
        if (
            signatures.length != nonces.length ||
            nonces.length != validityTimestamps.length ||
            validityTimestamps.length != values.length ||
            values.length != payloads.length
        ) {
            revert("Batch ExecuteRelayCall Params Length Mismatch");
        }

        bytes[] memory castedVotes = new bytes[](payloads.length);

        for (uint256 ii; ii < payloads.length; ++ii) {
            require(values[ii] == 0, "Batch entry cannot contain value");

            // cast each votes one by one
            castedVotes[ii] = executeRelayCall(
                signatures[ii],
                nonces[ii],
                validityTimestamps[ii],
                payloads[ii]
            );
        }

        return castedVotes;
    }

Version:

0.10.0

Relevant log output:

No response

@CJ42 CJ42 added the bug-candidate Bugs reports that are not yet confirmed label Nov 17, 2023
@0xalpharush
Copy link
Contributor

0xalpharush commented Nov 18, 2023

Thanks for this! Fwiw there is a dedicated issue template for false positives. We can likely update the detector to check if the use of msg.value is only in a conditional (require, assert, if-revert) and not detect those cases, but we may need to inspect whether it's being compared to zero (cc @smonicas )

def is_conditional(self, include_loop: bool = True) -> bool:

for ir in node.all_slithir_operations():
if in_loop_counter > 0 and SolidityVariableComposed("msg.value") in ir.read:
results.append(ir.node)
if isinstance(ir, (InternalCall)):
msg_value_in_loop(ir.function.entry_point, in_loop_counter, visited, results)

@0xalpharush 0xalpharush added false-positive and removed bug-candidate Bugs reports that are not yet confirmed labels Nov 18, 2023
@0xalpharush 0xalpharush changed the title [Bug-Candidate]: False positive with msg.value in a loop when msg.value is not transferred [False-Positive]:msg.value in a loop when msg.value is not transferred Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants