Skip to content

Commit

Permalink
Merge pull request #304 from CoinFabrik/migrate-unprotected-update-cu…
Browse files Browse the repository at this point in the history
…rrent-contract-wasm-documentation

New unprotected-update-current-contract-wasm documentation
  • Loading branch information
matiascabello authored Aug 14, 2024
2 parents f274b85 + aa47f87 commit affa9a1
Showing 1 changed file with 31 additions and 13 deletions.
44 changes: 31 additions & 13 deletions docs/docs/detectors/6-unprotected-update-current-contract-wasm.md
Original file line number Diff line number Diff line change
@@ -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.

### 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);
Expand All @@ -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);
Expand All @@ -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.

0 comments on commit affa9a1

Please sign in to comment.