From 828b4cf017368cadc77457ad18ffbc775c7a00fd Mon Sep 17 00:00:00 2001
From: tomasavola <108414862+tomasavola@users.noreply.github.com>
Date: Wed, 7 Aug 2024 10:29:44 -0300
Subject: [PATCH] New insufficiently-random-values documentation

---
 .../5-insufficiently-random-values.md         | 66 ++++++++++++++-----
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/docs/docs/detectors/5-insufficiently-random-values.md b/docs/docs/detectors/5-insufficiently-random-values.md
index 62bac4c7..87812d6e 100644
--- a/docs/docs/detectors/5-insufficiently-random-values.md
+++ b/docs/docs/detectors/5-insufficiently-random-values.md
@@ -1,17 +1,25 @@
-# Insuficciently random values
+# Insufficiently random values
 
-### What it does
-Checks the usage of `ledger().timestamp()` or `ledger().sequence()` for generation of random numbers.
+## Description 
+
+- Category: `Block attributes`
+- Severity: `Critical`
+- Detector: [`insufficiently-random-values`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values)
+- Test Cases: [`insufficiently-random-values-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1) 
+
+Using block attributes like `timestamp` or `sequence` for random number generation in Soroban smart contracts is not recommended due to the predictability of these values. Block attributes are publicly visible and deterministic, making it easy for malicious actors to anticipate their values and manipulate outcomes to their advantage. It's important to use a source that is both unpredictable and external to the blockchain environment, reducing the potential for malicious exploitation.
+
+## Why is this bad? 
+
+Using `ledger().timestamp()` is not recommended because it could be potentially manipulated by validator, which might lead to potential problems. On the other hand, `ledger().sequence()` is publicly available. An attacker could predict the random number to be generated to manipulate the code and perform an attack on the contract.
 
-### Why is this bad?
-Using `ledger().timestamp()` is not recommended because it could be potentially manipulated by validator. On the other hand, `ledger().sequence()` is publicly available, an attacker could predict the random number to be generated.
 
-### Example
+## Issue example 
+
+Consider the following `Soroban` contract:
 
 ```rust
-#[contractimpl]
-impl Contract {
-    pub fn generate_random_value_timestamp(env: Env, max_val: u64) -> Result<u64, Error> {
+pub fn generate_random_value_timestamp(env: Env, max_val: u64) -> Result<u64, Error> {
         if max_val == 0 {
             Err(Error::MaxValZero)
         } else {
@@ -19,6 +27,7 @@ impl Contract {
             Ok(val)
         }
     }
+    
     pub fn generate_random_value_sequence(env: Env, max_val: u32) -> Result<u32, Error> {
         if max_val == 0 {
             Err(Error::MaxValZero)
@@ -27,25 +36,46 @@ impl Contract {
             Ok(val)
         }
     }
-}
+    
 ```
 
-Use instead:
+The issue lies in these functions use of blockchain-provided data like block timestamp and sequence number for pseudo-random number generation. This reliance on predictable blockchain data makes the generated values susceptible to manipulation by attackers.
+
+The code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example).
+
+
+## Remediated example
+
+Avoid using block attributes like `timestamp` or `sequence` for randomness generation, and consider using PRNG instead.
 
 ```rust
-#[contractimpl]
-impl Contract {
-    pub fn generate_random_value(env: Env, max_val: u64) -> Result<u64, Error> {
+
+  pub fn generate_random_value(env: Env, max_val: u64) -> Result<u64, Error> {
         if max_val == 0 {
             Err(Error::MaxValZero)
         } else {
-            let val = env.prng().u64_in_range(0..max_val);
+            let val = env.prng().gen_range(0..max_val);
             Ok(val)
         }
     }
-}
+        
 ```
 
-### Implementation
+The remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example).
+
+## How is it detected?
+
+Checks the usage of `ledger().timestamp()` or `ledger().sequence()` for generation of random numbers.
+
+
+## References
+
+- https://dasp.co/#item-6
+- https://blog.sigmaprime.io/solidity-security.html#SP-6
+- [SWC-120](https://swcregistry.io/docs/SWC-120)
+- [SWC-116](https://swcregistry.io/docs/SWC-116)
+- [Ethernaut: Coinflip](https://ethernaut.openzeppelin.com/level/0x4dF32584890A0026e56f7535d0f2C6486753624f)
+- [Slither: Weak PRNG](https://github.com/crytic/slither/wiki/Detector-Documentation#weak-PRNG)
+- [Slither: Dangerous usage of block.timestamp](https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp)
 
-The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values).
\ No newline at end of file
+