[Bug-Candidate]: Slither seems to report erroneous error for bits shifts opcodes in assembly/Yul (shl(...))

[CJ42]

Describe the issue:

I have the following contract code below, that use bit shifts to create a mask that can be used to compare if part of a byte32 value given as input matches with the same part of a bytes32 state variable retrieved from the contract storage.

See contract example below to reproduce the exact behaviour.
As you can see below, I use assembly to do the bitshift for efficiency and save gas.

When testing the function, it works fine and work as expected.

However, Slither reports me the following error: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-shift-in-assembly

What I do not understand is that according to the Yul specification in the Solidity docs, the order of parameters is correct.

Code example to reproduce the issue:

// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.0;

contract Playground {
    bytes32 allowedDataKey;

    constructor(bytes32 allowedDataKey_) {
        allowedDataKey = allowedDataKey_;
    }

    function setAllowedDataKey(bytes32 _allowedDataKey) public {
        allowedDataKey = _allowedDataKey;
    }

    function isAllowedDataKey(bytes32 _inputKey) public view returns (bool) {
        uint256 zeroBytesCount = _countTrailingZeroBytes(allowedDataKey);

        bytes32 mask;

        assembly {
            // use a bitmask to discard the last `n` bytes of the input key
            // so to compare only the relevant parts of each ERC725Y keys
            //
            // `n = zeroBytesCount`
            //
            // eg:
            //
            // allowed key = 0xcafecafecafecafecafecafecafecafe00000000000000000000000000000000
            //
            //                        compare this part
            //                 vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
            //   input key = 0xcafecafecafecafecafecafecafecafe00000000000000000000000011223344
            //
            //         &                                              discard this part
            //                                                 vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
            //        mask = 0xffffffffffffffffffffffffffffffff00000000000000000000000000000000
            //
            mask := shl(
                mul(8, zeroBytesCount),
                0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
            )
        }

        if (allowedDataKey == (_inputKey & mask)) {
            return true;
        } else {
            return false;
        }
    }

    function _countTrailingZeroBytes(bytes32 _dataKey) internal pure returns (uint256) {
        uint256 index = 31;

        // CHECK each bytes of the key, starting from the end (right to left)
        // skip each empty bytes `0x00` to find the first non-empty byte
        while (_dataKey[index] == 0x00) index--;

        return 32 - (index + 1);
    }
}

Version:

0.8.3 (latest)

Relevant log output:

This is the actual error that seems to be reported incorrectly

Playground.isAllowedDataKey(bytes32) (contracts/Playground.sol#15-49) contains an incorrect shift operation: mask = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff << 8 * zeroBytesCount (contracts/Playground.sol#38-41)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#shift-parameter-mixup

There are also these informational warnings

Playground.isAllowedDataKey(bytes32) (contracts/Playground.sol#15-49) uses assembly
        - INLINE ASM (contracts/Playground.sol#20-42)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage

Pragma version^0.8.0 (contracts/Playground.sol#2) allows old versions
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-versions-of-solidity

Parameter Playground.setAllowedDataKey(bytes32)._allowedDataKey (contracts/Playground.sol#11) is not in mixedCase
Parameter Playground.isAllowedDataKey(bytes32)._inputKey (contracts/Playground.sol#15) is not in mixedCase
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions

Variable Playground.setAllowedDataKey(bytes32)._allowedDataKey (contracts/Playground.sol#11) is too similar to Playground.constructor(bytes32).allowedDataKey_ (contracts/Playground.sol#7)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#variable-names-are-too-similar

setAllowedDataKey(bytes32) should be declared external:
        - Playground.setAllowedDataKey(bytes32) (contracts/Playground.sol#11-13)
isAllowedDataKey(bytes32) should be declared external:
        - Playground.isAllowedDataKey(bytes32) (contracts/Playground.sol#15-49)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external

[CJ42]

@montyly @evgeniuz any updates on this?

[maurelian]

I'm getting the same false positives on this detector.

My code:

https://github.com/ethereum-optimism/optimism/blob/73c2a574049cd0f2516031546f2204c9456521d5/packages/contracts-bedrock/contracts/libraries/rlp/RLPReader.sol#L224-L226

            assembly {
                firstByteOfContent := and(mload(add(ptr, 1)), shl(248, 0xff))
            }

Slither output:

RLPReader._decodeLength(RLPReader.RLPItem) (contracts/libraries/rlp/RLPReader.sol#186-316) contains an incorrect shift operation: firstByteOfContent = mload(uint256)(ptr + 1) & 0xff << 248 (contracts/libraries/rlp/RLPReader.sol#225)

What's most concerning is that this suggests there's a backwards translation being made into slithIR, which presumably means that this detector reliably WOULD NOT detect this error when it does occur.

[0xalpharush]

I am not sure I follow your comment about the backwards translation as in Yul 248 is the number of bits to shift 0xff by. Seen below it's the same as 255 << 248. This detector flags constants on the left hand side of the expression, in this case 255, as it may mean someone switched the arguments inadvertently. It should probably also check that the right hand side isn't constant since that may be commonly done when bit masking as your example shows.

contract REPL {
    Cheats internal constant vm = Cheats(address(uint160(uint256(keccak256("hevm cheat code")))));

    /// @notice REPL contract entry point
    function run() public {
        uint256 x;
        assembly {
            x := shl(248, 0xff)
        }
        uint256 y = 255 << 248;
    }
}

➜ x
Type: uint
├ Hex: 0xff00000000000000000000000000000000000000000000000000000000000000
└ Decimal: 115339776388732929035197660848497720713218148788040405586178452820382218977280
➜ y
Type: uint
├ Hex: 0xff00000000000000000000000000000000000000000000000000000000000000
└ Decimal: 115339776388732929035197660848497720713218148788040405586178452820382218977280