From bb627e0328fdfed9fcb4b6b96a4cfe1d3afa2094 Mon Sep 17 00:00:00 2001 From: h1kk4 Date: Tue, 6 Jun 2023 19:38:01 +0600 Subject: [PATCH 1/2] unsafe ierc20 usage detection --- .../security/unsafe-transfer-with-ierc20.sol | 37 +++++++++++++++ .../security/unsafe-transfer-with-ierc20.yaml | 45 +++++++++++++++++++ .../unsafe-transferfrom-with-ierc20.sol | 37 +++++++++++++++ .../unsafe-transferfrom-with-ierc20.yaml | 43 ++++++++++++++++++ 4 files changed, 162 insertions(+) create mode 100644 solidity/security/unsafe-transfer-with-ierc20.sol create mode 100644 solidity/security/unsafe-transfer-with-ierc20.yaml create mode 100644 solidity/security/unsafe-transferfrom-with-ierc20.sol create mode 100644 solidity/security/unsafe-transferfrom-with-ierc20.yaml diff --git a/solidity/security/unsafe-transfer-with-ierc20.sol b/solidity/security/unsafe-transfer-with-ierc20.sol new file mode 100644 index 0000000..addfdaa --- /dev/null +++ b/solidity/security/unsafe-transfer-with-ierc20.sol @@ -0,0 +1,37 @@ +pragma solidity >=0.7.4; + +contract Test { + function payment() public { + //ruleid: unsafe-transfer-with-ierc20 + IERC20(token0).transfer(msg.sender, refund); + } + + function payment2() public { + //ruleid: unsafe-transfer-with-ierc20 + require(IERC20(token0).transfer(msg.sender, refund),"error"); + } + + function payment3() public { + //ruleid: unsafe-transfer-with-ierc20 + bool success = IERC20(token0).transfer(msg.sender, refund); + } +} + +contract Test2 { + IERC20 token0 = address(0); + function payment() public { + //ruleid: unsafe-transfer-with-ierc20 + token0.transfer(msg.sender, refund); + } +} + +contract Test3 { + IERC20 token0; + constructor() { + token0 = address(0); + } + function payment() public { + //ruleid: unsafe-transfer-with-ierc20 + token0.transfer(msg.sender, refund); + } +} \ No newline at end of file diff --git a/solidity/security/unsafe-transfer-with-ierc20.yaml b/solidity/security/unsafe-transfer-with-ierc20.yaml new file mode 100644 index 0000000..121c26c --- /dev/null +++ b/solidity/security/unsafe-transfer-with-ierc20.yaml @@ -0,0 +1,45 @@ +rules: + - id: unsafe-transfer-with-ierc20 + metadata: + references: + - https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca + category: security + tags: + - IERC20 + - transfer + message: The majority of code that accepts ERC20 tokens still accepts some tokens even when they do not correctly implement the standard. Using IERC20 interface to work with that kind of token may lead to inablility to transfer funds from smart contract. + Use OpenZeppelin's SafeERC20's safeTransfer() instead of transfer() to prevent this issue. + languages: + - solidity + severity: WARNING + patterns: + - pattern: | + $VAR.transfer($TO, $AMOUNT); + - pattern-either: + - pattern-inside: | + contract $C { + ... + IERC20 $VAR = ...; + ... + function $F(...) { + ... + $VAR.transfer($TO, $AMOUNT); + ... + } + ... + } + - pattern-inside: | + contract $C { + ... + IERC20 $VAR; + ... + function $F(...) { + ... + $VAR.transfer($TO, $AMOUNT); + ... + } + ... + } + - pattern-inside: IERC20(...); + + \ No newline at end of file diff --git a/solidity/security/unsafe-transferfrom-with-ierc20.sol b/solidity/security/unsafe-transferfrom-with-ierc20.sol new file mode 100644 index 0000000..1b722fd --- /dev/null +++ b/solidity/security/unsafe-transferfrom-with-ierc20.sol @@ -0,0 +1,37 @@ +pragma solidity >=0.7.4; + +contract Test { + function payment() public { + //ruleid: unsafe-transferfrom-with-ierc20 + IERC20(token0).transferFrom(msg.sender, address(this), refund); + } + + function payment2() public { + //ruleid: unsafe-transferfrom-with-ierc20 + require(IERC20(token0).transferFrom(msg.sender, address(this), refund),"error"); + } + + function payment3() public { + //ruleid: unsafe-transferfrom-with-ierc20 + bool success = IERC20(token0).transferFrom(msg.sender, address(this), refund); + } +} + +contract Test2 { + IERC20 token0 = address(0); + function payment() public { + //ruleid: unsafe-transferfrom-with-ierc20 + token0.transferFrom(msg.sender, address(this), refund); + } +} + +contract Test3 { + IERC20 token0; + constructor() { + token0 = address(0); + } + function payment() public { + //ruleid: unsafe-transferfrom-with-ierc20 + token0.transferFrom(msg.sender, address(this), refund); + } +} \ No newline at end of file diff --git a/solidity/security/unsafe-transferfrom-with-ierc20.yaml b/solidity/security/unsafe-transferfrom-with-ierc20.yaml new file mode 100644 index 0000000..65d1f51 --- /dev/null +++ b/solidity/security/unsafe-transferfrom-with-ierc20.yaml @@ -0,0 +1,43 @@ +rules: + - id: unsafe-transferfrom-with-ierc20 + metadata: + references: + - https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca + category: security + tags: + - IERC20 + - transferFrom + message: The majority of code that accepts ERC20 tokens still accepts some tokens even when they do not correctly implement the standard. Using IERC20 interface to work with that kind of token may lead to inablility to transfer funds from smart contract. + Use OpenZeppelin's SafeERC20's safeTransferFrom() instead of transferFrom() to prevent this issue. + languages: + - solidity + severity: WARNING + patterns: + - pattern: | + $VAR.transferFrom($FROM, $TO, $AMOUNT); + - pattern-either: + - pattern-inside: | + contract $C { + ... + IERC20 $VAR = ...; + ... + function $F(...) { + ... + $VAR.transferFrom($FROM, $TO, $AMOUNT); + ... + } + ... + } + - pattern-inside: | + contract $C { + ... + IERC20 $VAR; + ... + function $F(...) { + ... + $VAR.transferFrom($FROM, $TO, $AMOUNT); + ... + } + ... + } + - pattern-inside: IERC20(...); \ No newline at end of file From 2d24eb78a0c366306ad7830bf6a36ca15a3f7741 Mon Sep 17 00:00:00 2001 From: h1kk4 Date: Tue, 6 Jun 2023 19:42:37 +0600 Subject: [PATCH 2/2] fix README --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 5c35967..153b4b4 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,8 @@ no-bidi-characters | Generic | The code must not contain any of Unicode Directio delegatecall-to-arbitrary-address | Generic | An attacker may perform delegatecall() to an arbitrary address. incorrect-use-of-blockhash | Generic | blockhash(block.number) and blockhash(block.number + N) always returns 0. accessible-selfdestruct | Generic | Contract can be destructed by anyone in $FUNC +unsafe-transferfrom-with-ierc20 | Generic | Use OpenZeppelin's SafeERC20's safeTransferFrom() instead of transferFrom() +unsafe-transfer-with-ierc20 | Generic | Use OpenZeppelin's SafeERC20's safeTransfer() instead of transfer() ## Gas Optimization Rules