From e03d675cb21d22af3d0837c1cd9bf24f41b64c21 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Tue, 23 Apr 2024 16:54:03 -0300 Subject: [PATCH 1/3] Add vulnerability description for zero-or-test-address --- docs/docs/vulnerabilities/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/docs/vulnerabilities/README.md b/docs/docs/vulnerabilities/README.md index 6b461635..a7943ee4 100644 --- a/docs/docs/vulnerabilities/README.md +++ b/docs/docs/vulnerabilities/README.md @@ -222,3 +222,12 @@ Modifying mappings with an arbitrary key given by the user could lead to uninten This vulnerability falls under the [Validations and error handling category](#vulnerability-categories) and assigned it a Critical severity. +### Zero or test address + +The assignment of the zero address to a variable in a smart contract represents a critical vulnerability because it can lead to loss of control over the contract. This stems from the fact that the zero address does not have an associated private key, which means it's impossible to claim ownership, rendering any contract assets or functions permanently inaccessible. + +Assigning a test address can also have similar implications, including the loss of access or granting access to a malicious actor if its private keys are not handled with care. + +This vulnerability falls under the [Validations and error handling](#vulnerability-categories) category +and has a Medium severity. + From 38b0cbcab8a6a523536a9600be761527a5e5e387 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Tue, 23 Apr 2024 17:03:19 -0300 Subject: [PATCH 2/3] Add detector documentation --- docs/docs/detectors/17-zero-or-test-address | 43 +++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 docs/docs/detectors/17-zero-or-test-address diff --git a/docs/docs/detectors/17-zero-or-test-address b/docs/docs/detectors/17-zero-or-test-address new file mode 100644 index 00000000..b3b715bf --- /dev/null +++ b/docs/docs/detectors/17-zero-or-test-address @@ -0,0 +1,43 @@ +# Zero or test address + +### What it does +Checks whether the zero address is being inputed to a function without validation. + +### Why is this bad? +Because the private key for the zero address is known, anyone could take ownership of the contract. + +### Example + +```rust +pub fn set(e: Env, admin: Address, data: i32) -> Result<(), Error> { + if !ZeroAddressContract::ensure_is_admin(&e, admin)? { + return Err(Error::NotAdmin); + } + e.storage().persistent().set(&DataKey::Data, &data); + Ok(()) +} +``` + + +Use instead: +```rust +pub fn set(e: Env, admin: Address, data: i32) -> Result<(), Error> { + if admin + == Address::from_string(&String::from_bytes( + &e, + b"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", + )) + { + return Err(Error::InvalidNewAdmin); + } + if !ZeroAddressContract::ensure_is_admin(&e, admin)? { + return Err(Error::NotAdmin); + } + e.storage().persistent().set(&DataKey::Data, &data); + Ok(()) +} +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/zero-or-test-address). From 20f13920fea1da056f995f279c9367a02c12ead6 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Tue, 23 Apr 2024 17:06:42 -0300 Subject: [PATCH 3/3] Add detector line to detectors table --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 60318707..0c0b37cd 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,8 @@ Visit [Scout's website](https://coinfabrik.github.io/scout-soroban/) to view the | [iterators-over-indexing](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing) |Iterating with hardcoded indexes is slower than using an iterator. Also, if the index is out of bounds, it will panic. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing-1) | Enhancement | | [assert-violation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation) | Avoid the usage of the macro assert!, it can panic.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1) | Enhancement | | [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation) | Modifying mappings with an arbitrary key given by users can be a significant vulnerability. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-2) | Critical | +| [zero-or-test-address](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/zero-or-test-address) | Avoid zero or test address assignment to prevent contract control loss. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/zero-or-test-address/zero-or-test-address-1) | Validations and error handling | + ## Tests