From 8ec77324f87c49c37e771125c7bb384260617066 Mon Sep 17 00:00:00 2001 From: tomasavola <108414862+tomasavola@users.noreply.github.com> Date: Wed, 7 Aug 2024 10:38:42 -0300 Subject: [PATCH 1/2] New unprotected-update-current-contract-wasm documentation --- ...nprotected-update-current-contract-wasm.md | 44 +++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md b/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md index 94824131..bac1e8a7 100644 --- a/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md +++ b/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md @@ -1,17 +1,26 @@ -# Unprotected update of current contract wasm +# Unprotected update current contract wasm -### What it does +## Description -It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. +- Category: `Authorization` +- Severity: `Critical` +- Detector: [`unprotected-update-current-contract-wasm`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) +- Test Cases: [`unprotected-update-current-contract-wasm-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) [`unprotected-update-current-contract-wasm-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-2) + +It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. 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. -### Why is this bad? + +## Why is this bad? 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. -### Example +## Issue example + +Consider the following `Soroban` contract: ```rust -#[contractimpl] + + #[contractimpl] impl UpgradeableContract { pub fn init(e: Env, admin: Address) { e.storage().instance().set(&DataKey::Admin, &admin); @@ -22,17 +31,24 @@ impl UpgradeableContract { } pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { - let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap(); - e.deployer().update_current_contract_wasm(new_wasm_hash); } } -``` -Use instead: +``` + +This contract allows upgrades through the `update_current_contract_wasm` function. If just anyone can call this function, they could modify the contract behaviour. + +The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example). + + +## Remediated example + +To prevent this, the function should be restricted to administrators or authorized users only. ```rust -#[contractimpl] + + #[contractimpl] impl UpgradeableContract { pub fn init(e: Env, admin: Address) { e.storage().instance().set(&DataKey::Admin, &admin); @@ -51,6 +67,8 @@ impl UpgradeableContract { } ``` -### Implementation +The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example). -The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) +## How is it detected? + +It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. From aa47f874cf155b675529b386eaba904f8677c8dd Mon Sep 17 00:00:00 2001 From: tomasavola <108414862+tomasavola@users.noreply.github.com> Date: Mon, 12 Aug 2024 09:43:40 -0300 Subject: [PATCH 2/2] Remove "If users are (...)" --- .../detectors/6-unprotected-update-current-contract-wasm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md b/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md index bac1e8a7..6b4e5f4d 100644 --- a/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md +++ b/docs/docs/detectors/6-unprotected-update-current-contract-wasm.md @@ -7,7 +7,7 @@ - Detector: [`unprotected-update-current-contract-wasm`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) - Test Cases: [`unprotected-update-current-contract-wasm-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) [`unprotected-update-current-contract-wasm-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-2) -It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. 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. +It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. ## Why is this bad?