Skip to content

Commit

Permalink
Fix Overzealous Zero Address Checker Detector (#563)
Browse files Browse the repository at this point in the history
  • Loading branch information
alexroan authored Jun 24, 2024
1 parent 7d22843 commit 552d36f
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 34 deletions.
18 changes: 15 additions & 3 deletions aderyn_core/src/detect/low/zero_address_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
);
Expand Down
26 changes: 1 addition & 25 deletions reports/ccip-functions-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

<details><summary>10 Found Instances</summary>
<details><summary>6 Found Instances</summary>


- 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)
Expand All @@ -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
Expand Down
18 changes: 16 additions & 2 deletions reports/report.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"files_summary": {
"total_source_units": 40,
"total_sloc": 1260
"total_source_units": 41,
"total_sloc": 1322
},
"files_details": {
"files_details": [
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
21 changes: 17 additions & 4 deletions reports/report.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 |
Expand All @@ -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
Expand Down Expand Up @@ -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.

<details><summary>3 Found Instances</summary>
<details><summary>5 Found Instances</summary>


- 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)

Expand Down
22 changes: 22 additions & 0 deletions reports/report.sarif
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
77 changes: 77 additions & 0 deletions tests/contract-playground/src/TestERC20.sol
Original file line number Diff line number Diff line change
@@ -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];
}

}

0 comments on commit 552d36f

Please sign in to comment.