add bad-transferfrom-access-control rule

Decurity · Aug 15, 2024 · 5d1079a · 5d1079a
1 parent 75073c2
commit 5d1079a
Show file tree

Hide file tree

Showing 3 changed files with 226 additions and 1 deletion.
diff --git a/README.md b/README.md
@@ -137,7 +137,8 @@ compound-precision-loss | Hundred Finance, Midas Finance, Onyx Protocol | In Com
 thirdweb-vulnerability | Swopple Token, TIME Token, NAME Token, HXA Token | In contracts that support Multicall and ERC2771Context an Arbitrary Address Spoofing attack is possible
 exact-balance-check | Generic | Testing the balance of an account as a basis for some action has risks associated with unexpected receipt of ether or another token, including tokens deliberately transfered to cause such tests to fail, as an MEV attack.
 missing-assignment | Generic | Meaningless statement that does not change any values could be a sign of missed security checks or other important changes.
-oracle-uses-curve-spot-price | UwU | Oracle uses the get_p() Curve pool function which can be manipulated via flashloan to calculate the asset price 
+oracle-uses-curve-spot-price | UwU | Oracle uses the get_p() Curve pool function which can be manipulated via flashloan to calculate the asset price
+bad-transferfrom-access-control | Generic | Funds approved by users can be stolen because of improper access control to a transferFrom function 
 
 ## Gas Optimization Rules
 

diff --git a/solidity/security/bad-transferfrom-access-control.sol b/solidity/security/bad-transferfrom-access-control.sol
@@ -0,0 +1,181 @@
+contract Test {
+    function func1(address from, address to) public {
+        // ruleid: bad-transferfrom-access-control
+        usdc.transferFrom(from, to, amount);
+    }
+
+    function func2(address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(owner, random, amount);
+    }
+
+    function func3(address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(pool, to, amount);
+    }
+
+    function func4(address from, uint256 amount, address random) public {
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(pool, owner, amount);
+    }
+
+    function func5(address from, address to) external {
+        // ruleid: bad-transferfrom-access-control
+        usdc.transferFrom(from, to, amount);
+    }
+
+    function func6(address from, address to) external {
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(owner, random, amount);
+    }
+
+    function func7(address from, address to) external {
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(pool, to, amount);
+    }
+
+    function func8(address from, uint256 amount, address random) external {
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(pool, owner, amount);
+    }
+
+    function transferFee(uint256 amount, uint256 feeBps, address token, address from, address to)
+        public
+        returns (uint256)
+    {
+        uint256 fee = calculateFee(amount, feeBps);
+        if (fee > 0) {
+            if (token != NATIVE_TOKEN) {
+                // ERC20 token
+                if (from == address(this)) {
+                    TransferHelper.safeTransfer(token, to, fee);
+                } else {
+                    // safeTransferFrom requires approval
+                    // ruleid: bad-transferfrom-access-control
+                    TransferHelper.transferFrom(token, from, to, fee);
+                }
+            } else {
+                require(from == address(this), "can only transfer eth from the router address");
+
+                // Native ether
+                (bool success,) = to.call{value: fee}("");
+                require(success, "transfer failed in transferFee");
+            }
+            return fee;
+        } else {
+            return 0;
+        }
+    }
+
+    function func9(address from, address to) external {
+        _func10(from, to, amount);
+    }
+
+    function _func10(address from, address to) internal {
+        // ruleid: bad-transferfrom-access-control
+        usdc.transferFrom(from, to, amount);
+    }
+
+
+    // SAFE TRANSFER TESTS
+
+    function func11(address from, address to) public {
+        // ruleid: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, to, amount);
+    }
+
+    function func12(address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(owner, random, amount);
+    }
+
+    function func13(address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(pool, to, amount);
+    }
+
+    function func14(address from, uint256 amount, address random) public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(pool, owner, amount);
+    }
+
+    function func15(address from, address to) external {
+        // ruleid: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, to, amount);
+    }
+
+    function func16(address from, address to) external {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(owner, random, amount);
+    }
+
+    function func17(address from, address to) external {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(pool, to, amount);
+    }
+
+    function func18(address from, uint256 amount, address random) external {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(pool, owner, amount);
+    }
+
+    function transferFee2(uint256 amount, uint256 feeBps, address token, address from, address to)
+        public
+        returns (uint256)
+    {
+        uint256 fee = calculateFee(amount, feeBps);
+        if (fee > 0) {
+            if (token != NATIVE_TOKEN) {
+                // ERC20 token
+                if (from == address(this)) {
+                    TransferHelper.safeTransfer(token, to, fee);
+                } else {
+                    // safeTransferFrom requires approval
+                    // ruleid: bad-transferfrom-access-control
+                    TransferHelper.safeTransferFrom(token, from, to, fee);
+                }
+            } else {
+                require(from == address(this), "can only transfer eth from the router address");
+
+                // Native ether
+                (bool success,) = to.call{value: fee}("");
+                require(success, "transfer failed in transferFee");
+            }
+            return fee;
+        } else {
+            return 0;
+        }
+    }
+
+    function func19(address from, address to) external {
+        _func20(from, to, amount);
+    }
+
+    function _func20(address from, address to) internal {
+        // ruleid: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, to, amount);
+    }
+
+    function _func21(address from, address to) internal {
+        // internal never called
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, to, amount);
+        // ok: bad-transferfrom-access-control
+        usdc.transferFrom(from, to, amount);
+        // ok: bad-transferfrom-access-control
+        Helper.safeTransferFrom(token, from, to, amount);
+        // ok: bad-transferfrom-access-control
+        Helper.transferFrom(token, from, to, amount);
+    }
+
+    function func22(address from, address to) external {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, from, amount);
+    }
+
+    function func23(address to, address from) external {
+        // ruleid: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, to, amount);
+    }
+
+}
diff --git a/solidity/security/bad-transferfrom-access-control.yaml b/solidity/security/bad-transferfrom-access-control.yaml
@@ -0,0 +1,43 @@
+rules:
+  - id: bad-transferfrom-access-control
+    languages:
+      - solidity
+    severity: ERROR
+    message: An attacker may perform transferFrom() with arbitrary addresses
+    metadata:
+      category: security
+      technology:
+        - solidity
+      cwe: "CWE-284: Improper Access Control"
+      confidence: LOW
+      likelihood: HIGH
+      impact: HIGH
+      subcategory:
+        - vuln
+      references: 
+        - https://app.blocksec.com/explorer/tx/eth/0x54f659773dae6e01f83184d4b6d717c7f1bb71c0aa59e8c8f4a57c25271424b3 # YODL hack
+    mode: taint
+    pattern-sources:
+      - label: INPUT_TO
+        pattern-either:
+          - patterns:
+              - pattern: function $F(..., address $FROM, ..., address $TO, ...) public { ... }
+              - focus-metavariable: $TO
+          - patterns:
+              - pattern: function $F(..., address $FROM, ..., address $TO, ...) external { ... }
+              - focus-metavariable: $TO
+      - label: INPUT_FROM
+        pattern-either:
+          - patterns:
+              - pattern: function $F(..., address $FROM, ..., address $TO, ...) public { ... }
+              - focus-metavariable: $FROM
+          - patterns:
+              - pattern: function $F(..., address $FROM, ..., address $TO, ...) external { ... }
+              - focus-metavariable: $FROM
+    pattern-sinks:
+      - requires: INPUT_TO and INPUT_FROM
+        pattern-either:
+          - pattern: $TOKEN.transferFrom($FROM,$TO,$AMOUNT);
+          - pattern: $TOKEN.safeTransferFrom($FROM,$TO,$AMOUNT);
+          - pattern: $HELPER.transferFrom($TOKEN,$FROM,$TO,...);
+          - pattern: $HELPER.safeTransferFrom($TOKEN,$FROM,$TO,...);