Decurity/semgrep-smart-contracts

Fix: arbitrary-low-level-call matching workarounds

plotchy opened this issue · 1 comments

Great repo. I've been learning semgrep with your examples and believe I've found a workaround to incorporate arbitrary-low-level-calls without waiting for beta improvements to solidity semgrep.

The fixes include:

  • Defining types for metavariables inside function declarations
  • Adding pattern-either patterns to include curly braces for call
rules:
 -
    id: arbitrary-low-level-call
    message: An attacker may perform call() to an arbitrary address with controlled calldata
    metadata:
        references:
        - https://twitter.com/CertiKAlert/status/1512198846343954445
        - https://twitter.com/SlowMist_Team/status/1508787862791069700
        - https://twitter.com/Beosin_com/status/1509099103401127942
        - https://blocksecteam.medium.com/li-fi-attack-a-cross-chain-bridge-vulnerability-no-its-due-to-unchecked-external-call-c31e7dadf60f
        - https://etherscan.io/address/0xe7597f774fd0a15a617894dc39d45a28b97afa4f # Auctus Options
        - https://etherscan.io/address/0x73a499e043b03fc047189ab1ba72eb595ff1fc8e # Li.Fi
        category: controlled-call
        tags:
        - auctus-options
        - starstream-finance
        - basket-dao
        - li-finance
    patterns:
    - pattern-either:
        - pattern-inside: |
            function $F(..., address $ADDR, ..., bytes calldata $DATA, ...) external { ... }
        - pattern-inside: |
            function $F(..., address $ADDR, ..., bytes calldata $DATA, ...) public { ... }
    - pattern-either:
        - pattern: $ADDR.call($DATA);
        - pattern: $ADDR.call{$VALUE:...}($DATA);
        - pattern: $ADDR.call{$VALUE:..., $GAS:...}($DATA);
    languages: 
    - solidity
    severity: ERROR

Four matching cases:

contract wxBTRFLY is FrozenToken {
    function execute(
        address to,
        uint256 value,
        bytes calldata data
    ) external returns (bool, bytes memory) {

        // ruleid: arbitrary-low-level-call
        (bool success, bytes memory result) = to.call{value: value}(data);

        // ruleid: arbitrary-low-level-call
        (bool success, bytes memory result) = to.call{gas: value}(data);

        // ruleid: arbitrary-low-level-call
        (bool success, bytes memory result) = to.call(data);

        // ruleid: arbitrary-low-level-call
        (bool success, bytes memory result) = to.call{value: value, gas: 0}(data);

        return (success, result);
    }
}
Raz0r commented

Hey @plotchy, thank you for this info. Fixed one more rule by adding argument types. BTW a new semgrep has been released with a better Solidity parser (https://github.com/returntocorp/semgrep/releases/tag/v0.100.0), will test remaining unfixed rules with it.