From 552d36f4f284f9935571417b6d44f3e4783f4aff Mon Sep 17 00:00:00 2001 From: Alex Roan Date: Mon, 24 Jun 2024 14:12:08 +0100 Subject: [PATCH] Fix Overzealous Zero Address Checker Detector (#563) --- .../src/detect/low/zero_address_check.rs | 18 ++++- reports/ccip-functions-report.md | 26 +------ reports/report.json | 18 ++++- reports/report.md | 21 ++++- reports/report.sarif | 22 ++++++ tests/contract-playground/src/TestERC20.sol | 77 +++++++++++++++++++ 6 files changed, 148 insertions(+), 34 deletions(-) create mode 100644 tests/contract-playground/src/TestERC20.sol diff --git a/aderyn_core/src/detect/low/zero_address_check.rs b/aderyn_core/src/detect/low/zero_address_check.rs index c32e14669..a176ef401 100644 --- a/aderyn_core/src/detect/low/zero_address_check.rs +++ b/aderyn_core/src/detect/low/zero_address_check.rs @@ -39,13 +39,13 @@ impl IssueDetector for ZeroAddressCheckDetector { .type_string .as_deref() .unwrap_or("") - .contains("address") + == "address" || var_decl .type_descriptions .type_string .as_deref() .unwrap_or("") - .contains("contract")) + .contains("contract ")) { Some((var_decl.id, (*var_decl).clone())) // Deref and clone the VariableDeclaration. } else { @@ -199,7 +199,19 @@ mod zero_address_check_tests { #[test] #[serial] - fn test_deprecated_oz_functions_detector_by_loading_contract_directly() { + fn test_zero_address_check_using_mapping_with_address_in_it() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/TestERC20.sol", + ); + let mut detector = ZeroAddressCheckDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that nothing was found + assert!(!found); + } + + #[test] + #[serial] + fn test_zero_address_check_detector_by_loading_contract_directly() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/ZeroAddressCheck.sol", ); diff --git a/reports/ccip-functions-report.md b/reports/ccip-functions-report.md index 5ea121afd..517c60d8c 100644 --- a/reports/ccip-functions-report.md +++ b/reports/ccip-functions-report.md @@ -742,7 +742,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid Check for `address(0)` when assigning values to address state variables. -
10 Found Instances +
6 Found Instances - Found in src/v0.8/functions/dev/v1_X/FunctionsBilling.sol [Line: 78](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol#L78) @@ -757,36 +757,12 @@ Check for `address(0)` when assigning values to address state variables. s_linkToUsdFeed = AggregatorV3Interface(linkToUsdFeed); ``` -- Found in src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol [Line: 133](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol#L133) - - ```solidity - s_withdrawableTokens[msg.sender] += costWithoutCallbackJuels + callbackGasCostJuels; - ``` - -- Found in src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol [Line: 136](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol#L136) - - ```solidity - s_withdrawableTokens[address(this)] += adminFee; - ``` - - Found in src/v0.8/functions/v1_0_0/FunctionsBilling.sol [Line: 73](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/FunctionsBilling.sol#L73) ```solidity s_linkToNativeFeed = AggregatorV3Interface(linkToNativeFeed); ``` -- Found in src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol [Line: 133](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol#L133) - - ```solidity - s_withdrawableTokens[msg.sender] += costWithoutCallbackJuels + callbackGasCostJuels; - ``` - -- Found in src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol [Line: 136](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol#L136) - - ```solidity - s_withdrawableTokens[address(this)] += adminFee; - ``` - - Found in src/v0.8/functions/v1_1_0/FunctionsBilling.sol [Line: 85](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_1_0/FunctionsBilling.sol#L85) ```solidity diff --git a/reports/report.json b/reports/report.json index 19ca5eacd..7ec298e13 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 40, - "total_sloc": 1260 + "total_source_units": 41, + "total_sloc": 1322 }, "files_details": { "files_details": [ @@ -89,6 +89,10 @@ "file_path": "src/T11sTranferer.sol", "n_sloc": 8 }, + { + "file_path": "src/TestERC20.sol", + "n_sloc": 62 + }, { "file_path": "src/UnprotectedInitialize.sol", "n_sloc": 25 @@ -1287,6 +1291,16 @@ "description": "Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.", "detector_name": "unindexed-events", "instances": [ + { + "contract_path": "src/TestERC20.sol", + "line_no": 14, + "src": "338:70" + }, + { + "contract_path": "src/TestERC20.sol", + "line_no": 15, + "src": "413:70" + }, { "contract_path": "src/eth2/DepositContract.sol", "line_no": 19, diff --git a/reports/report.md b/reports/report.md index 89dbfa7e8..262bd4b7d 100644 --- a/reports/report.md +++ b/reports/report.md @@ -46,8 +46,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 40 | -| Total nSLOC | 1260 | +| .sol Files | 41 | +| Total nSLOC | 1322 | ## Files Details @@ -75,6 +75,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/StateVariables.sol | 58 | | src/StorageConditionals.sol | 59 | | src/T11sTranferer.sol | 8 | +| src/TestERC20.sol | 62 | | src/UnprotectedInitialize.sol | 25 | | src/UnsafeERC721Mint.sol | 18 | | src/UnusedError.sol | 19 | @@ -94,7 +95,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/parent_chain/ParentChainContract.sol | 29 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **1260** | +| **Total** | **1322** | ## Issue Summary @@ -1476,8 +1477,20 @@ If the same constant literal value is used multiple times, create a constant sta Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. -
3 Found Instances +
5 Found Instances + + +- Found in src/TestERC20.sol [Line: 14](../tests/contract-playground/src/TestERC20.sol#L14) + ```solidity + event Approval(address indexed src, address indexed usr, uint256 wad); + ``` + +- Found in src/TestERC20.sol [Line: 15](../tests/contract-playground/src/TestERC20.sol#L15) + + ```solidity + event Transfer(address indexed src, address indexed dst, uint256 wad); + ``` - Found in src/eth2/DepositContract.sol [Line: 19](../tests/contract-playground/src/eth2/DepositContract.sol#L19) diff --git a/reports/report.sarif b/reports/report.sarif index 9b811d4d4..959a02b9e 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2342,6 +2342,28 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TestERC20.sol" + }, + "region": { + "charLength": 70, + "charOffset": 338 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TestERC20.sol" + }, + "region": { + "charLength": 70, + "charOffset": 413 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/TestERC20.sol b/tests/contract-playground/src/TestERC20.sol new file mode 100644 index 000000000..287869d5b --- /dev/null +++ b/tests/contract-playground/src/TestERC20.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity 0.8.19; + +contract TestERC20 { + + uint256 public constant decimals = 18; + string public name; + string public symbol; + uint256 public totalSupply; + + mapping (address => uint256) public balances; + mapping (address => mapping (address => uint256)) private allowances; + + event Approval(address indexed src, address indexed usr, uint256 wad); + event Transfer(address indexed src, address indexed dst, uint256 wad); + + function getChainId() external view returns(uint256) { + uint256 chainId; + assembly { + chainId := chainid() + } + return chainId; + } + + constructor(string memory _symbol, string memory _name) { + symbol = _symbol; + name = _name; + } + + function transfer(address dst, uint256 wad) external returns (bool) { + return transferFrom(msg.sender, dst, wad); + } + + function transferFrom(address src, address dst, uint256 wad) public returns (bool) { + require(balances[src] >= wad, "nsufficient-balance"); + if (src != msg.sender && allowances[src][msg.sender] != type(uint256).max) { + require(allowances[src][msg.sender] >= wad, "insufficient-allowances"); + allowances[src][msg.sender] -= wad; + } + balances[src] -= wad; + balances[dst] += wad; + emit Transfer(src, dst, wad); + return true; + } + + function mint(address usr, uint256 wad) external { + balances[usr] += wad; + totalSupply += wad; + emit Transfer(address(0), usr, wad); + } + + function burn(address usr, uint256 wad) external { + require(balances[usr] >= wad, "insufficient-balance"); + if (usr != msg.sender && allowances[usr][msg.sender] != type(uint256).max) { + require(allowances[usr][msg.sender] >= wad, "insufficient-allowances"); + allowances[usr][msg.sender] -= wad; + } + balances[usr] -= wad; + totalSupply -= wad; + emit Transfer(usr, address(0), wad); + } + + function approve(address usr, uint256 wad) external returns (bool) { + allowances[msg.sender][usr] = wad; + emit Approval(msg.sender, usr, wad); + return true; + } + + function allowance(address owner, address spender) external view returns (uint256) { + return allowances[owner][spender]; + } + + function balanceOf(address account) external view returns (uint256) { + return balances[account]; + } + +} \ No newline at end of file