add bad-transferfrom-access-control rule (#67)

* add bad-transferfrom-access-control rule * update bad-transferfrom-access-control rule and tests * update rule and tests
Decurity · Aug 29, 2024 · ffb49be · ffb49be
1 parent 75073c2
commit ffb49be
Show file tree

Hide file tree

Showing 3 changed files with 284 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,242 @@
+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 {
+        // todoruleid: 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 {
+        // todoruleid: 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);
+    }
+
+    function transferFrom(address to, address from, uint256 amount) external {
+        // ok: bad-transferfrom-access-control
+        super.transferFrom(from, to, amount);
+    }
+
+    function stakeForAccount(address _fundingAccount, address _account, address _depositToken, uint256 _amount) external override nonReentrant {
+        _validateHandler();
+        _stake(_fundingAccount, _account, _depositToken, _amount);
+    }
+
+    function _stake(address _fundingAccount, address _account, address _depositToken, uint256 _amount) private {
+        require(_amount > 0, "RewardTracker: invalid _amount");
+        require(isDepositToken[_depositToken], "RewardTracker: invalid _depositToken");
+
+        // ok: bad-transferfrom-access-control
+        IERC20(_depositToken).transferFrom(_fundingAccount, address(this), _amount);
+
+        _updateRewards(_account);
+
+        stakedAmounts[_account] = stakedAmounts[_account] + _amount;
+        depositBalances[_account][_depositToken] = depositBalances[_account][_depositToken] + _amount;
+        totalDepositSupply[_depositToken] = totalDepositSupply[_depositToken] + _amount;
+
+        _mint(_account, _amount);
+    }
+
+
+    function func24(address from, address to) onlyOwner public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, to, amount);
+    }
+
+    function func25(address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, address(this), amount);
+    }
+
+    function transferIn(
+    address _token,
+    address _sender,
+    uint256 _amount
+    ) public onlyGame onlyWhitelistedToken(_token) {
+        // ok: bad-transferfrom-access-control
+        IERC20(_token).safeTransferFrom(_sender, address(this), _amount);
+    }
+
+    function func26(address random, address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(from, someaddress, amount);
+    }
+
+    function func28(address random, address from, address to) public {
+        // ok: bad-transferfrom-access-control
+        usdc.safeTransferFrom(this, some, from);
+    }
+
+    function func29(address random, address from, address token, address onemore) public {
+        // ok: bad-transferfrom-access-control
+        IERC20(token).safeTransferFrom(this, some, amount);
+    }
+}
+
diff --git a/solidity/security/bad-transferfrom-access-control.yaml b/solidity/security/bad-transferfrom-access-control.yaml
@@ -0,0 +1,40 @@
+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
+    options:
+      taint_unify_mvars: true
+    pattern-sources:
+      - patterns:
+          - pattern-either:
+              - pattern: function $F(..., address $FROM, ..., address $TO, ...) external { ... }
+              - pattern: function $F(..., address $FROM, ..., address $TO, ...) public { ... }
+              - pattern: function $F(..., address $TO, ..., address $FROM, ...) external { ... }
+              - pattern: function $F(..., address $TO, ..., address $FROM, ...) public { ... }
+          - focus-metavariable:
+              - $FROM
+              - $TO
+          - pattern-not: function $F(...) onlyOwner { ... }
+    pattern-sinks:
+      - patterns:
+          - 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,...);
+          - pattern-not: super.$FUN(...);