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..f73555b --- /dev/null +++ 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); + } + +} \ No newline at end of file diff --git a/solidity/security/bad-transferfrom-access-control.yaml b/solidity/security/bad-transferfrom-access-control.yaml new file mode 100644 index 0000000..894009d --- /dev/null +++ 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,...);