From ffb49be2ea85a47de589939b9f5a5df0092fabd5 Mon Sep 17 00:00:00 2001 From: Andrey <79048807+morsiiik@users.noreply.github.com> Date: Thu, 29 Aug 2024 11:13:32 +0300 Subject: [PATCH] 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 --- README.md | 3 +- .../bad-transferfrom-access-control.sol | 242 ++++++++++++++++++ .../bad-transferfrom-access-control.yaml | 40 +++ 3 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 solidity/security/bad-transferfrom-access-control.sol create mode 100644 solidity/security/bad-transferfrom-access-control.yaml diff --git a/README.md b/README.md index 27fd5bd..a4c16c7 100644 --- 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 new file mode 100644 index 0000000..569a439 --- /dev/null +++ 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 new file mode 100644 index 0000000..549b81a --- /dev/null +++ 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(...);