From 85c8560d3a13383190a709e53ba532e9748ba036 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Dec 2023 16:59:48 -0300 Subject: [PATCH 1/4] Add set contract storage documentation --- test-cases/README.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index b2956151..779e7d20 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -118,3 +118,27 @@ and has a Critical severity. If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it. To prevent this, the function should be restricted to administrators or authorized users only. This vulnerability falls under the [Authorization](#vulnerability-categories) category and has a Critical severity. + + +### Set contract storage + +Smart contracts can store important information in memory which changes through the contract's lifecycle. Changes happen via user interaction with the smart contract. An _unauthorized_ set contract storage vulnerability happens when a smart contract call allows a user to set or modify contract memory when they were not supposed to be authorized. + +Common practice is to have functions with the ability to change +security-relevant values in memory to be only accessible to specific roles, +e.g, only an admin can call the function `reset()` which resets auction values. +When this does not happen, arbitrary users may alter memory which may impose +great damage to the smart contract users. + +In `Soroban`, the method `env.storage()` can be used +to modify the contract storage under a given key. When a smart contract uses +this method, the contract needs to check if the caller should be able to +alter this storage. If this does not happen, an arbitary caller may modify +balances and other relevant contract storage. + +We classified this type of vulnerability under +the [Authorization](#vulnerability-categories) category and assigned it a +Critical severity. + + + From 5dea0419d111d6defc673dab95569f7040675bca Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Dec 2023 17:26:21 -0300 Subject: [PATCH 2/4] Create README.md --- detectors/set-contract-storage/README.md | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 detectors/set-contract-storage/README.md diff --git a/detectors/set-contract-storage/README.md b/detectors/set-contract-storage/README.md new file mode 100644 index 00000000..85350694 --- /dev/null +++ b/detectors/set-contract-storage/README.md @@ -0,0 +1,34 @@ +# Set contract storage + +### What it does + +Checks for calls to `env.storage()` without a prior call to `env.require_auth()`. + +### Why is this bad? + +Functions using keys as variables without proper access control or input sanitation can allow users to perform changes in arbitrary memory locations. + +### Known problems + +Only check the function call, so false positives could result. + +### Example + +```rust +fn set_contract_storage(env: Env) { + let _storage = env.storage().instance(); +} +``` + +Use instead: + +```rust +fn set_contract_storage(env: Env, user: Address) { + user.require_auth(); + let _storage = env.storage().instance(); +} +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage). From 4eb9934e2355f4b0d70cb2c4073ada17a8a2e87c Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Dec 2023 17:31:59 -0300 Subject: [PATCH 3/4] Update README.md Added references to avoid core mem forget and set contract storage detectors and test cases --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 09e8b4df..a6e94508 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,8 @@ cargo scout-audit | [overflow-check](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check) | An arithmetic operation overflows or underflows the available memory allocated to the variable. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/overflow-check/overflow-check-1)| Critical | | [insufficiently-random-values](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values) | Avoid using block attributes for random number generation to prevent manipulation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1)| Critical | | [unprotected-update-current-contract-wasm](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) | If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1)| Critical | +| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget) | The use of `core::mem::forget()` could lead to memory leaks and logic errors. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) +| [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage) | Insufficient access control on `env.storage()` method. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3) | Critical | ## Tests From 14e9fe58bcd901010209244b73403d659c02bf18 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Fri, 15 Dec 2023 11:40:14 -0300 Subject: [PATCH 4/4] Update README.md --- test-cases/README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index 779e7d20..2c8e4b96 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -119,6 +119,12 @@ If users are allowed to call `update_current_contract_wasm()`, they can intentio This vulnerability falls under the [Authorization](#vulnerability-categories) category and has a Critical severity. +### Avoid core::mem::forget + +The `core::mem::forget` function is used to forget about a value without running its destructor. This could lead to memory leaks and logic errors. + +We classified this issue, a deviation from best practices which could have +security implications, under the [Best practices](#vulnerability-categories) category and assigned it an Enhancement severity. ### Set contract storage