Merge pull request #7 from futuretech6/docs/detector-spec

[Docs] Add md docs
blocksecteam · Jan 7, 2024 · 8ae0d13 · 8ae0d13
2 parents f0a03ac + 6574914
commit 8ae0d13
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 12 deletions.
diff --git a/README.md b/README.md
@@ -70,24 +70,24 @@ docker run --rm -it \
 
 ## Detector Spec
 
-| **Detector**            | **Description**                                                                          | **Severity** | **Confidence** |
-| ----------------------- | ---------------------------------------------------------------------------------------- | ------------ | -------------- |
-| `UniswapPublicHook`     | callers of hook functions are not exclusively<br />restricted to the pool manager alone  | High         | High           |
-| `UniswapPublicCallback` | callers of callback functions are not exclusively<br />restricted to the contract itself | High         | High           |
-| `UniswapUpgradableHook` | the contract `DELEGATECALL`s to mutable addresses                                        | High         | High           |
-| `UniswapSuicidalHook`   | the contract contains `SELFDESTRUCT`                                                     | Medium       | High           |
+| **Detector**                               | **Description**                                                                          | **Severity** | **Confidence** |
+| ------------------------------------------ | ---------------------------------------------------------------------------------------- | ------------ | -------------- |
+| [`UniswapPublicHook`][public_hook]         | callers of hook functions are not exclusively<br />restricted to the pool manager alone  | High         | High           |
+| [`UniswapPublicCallback`][public_callback] | callers of callback functions are not exclusively<br />restricted to the contract itself | High         | High           |
+| [`UniswapUpgradableHook`][upgradable_hook] | the contract `DELEGATECALL`s to mutable addresses                                        | High         | High           |
+| [`UniswapSuicidalHook`][suicidal_hook]     | the contract contains `SELFDESTRUCT`                                                     | Medium       | High           |
 
 ## Evaluation
 
 We've conducted tests on 13 hook contracts associated with Uniswap v4, as listed in the compilation [awesome-uniswap-hook](https://github.com/hyperoracle/awesome-uniswap-hooks), all of which compiled without errors.
 The test results are as follows:
 
-| **Detector**            | **TP/ground_truth** |
-| ----------------------- | ------------------- |
-| `UniswapPublicHook`     | 7/7 contracts       |
-| `UniswapPublicCallback` | 3/3 contracts       |
-| `UniswapUpgradableHook` | 0                   |
-| `UniswapSuicidalHook`   | 0                   |
+| **Detector**                               | **TP/ground_truth** |
+| ------------------------------------------ | ------------------- |
+| [`UniswapPublicHook`][public_hook]         | 7/7 contracts       |
+| [`UniswapPublicCallback`][public_callback] | 3/3 contracts       |
+| [`UniswapUpgradableHook`][upgradable_hook] | 0                   |
+| [`UniswapSuicidalHook`][suicidal_hook]     | 0                   |
 
 ## Note
 
@@ -100,3 +100,8 @@ To uncover and address these sophisticated semantic concerns, the expertise of B
 ## License
 
 This project is under the AGPLv3 License. See the LICENSE file for the full license text.
+
+[public_callback]: docs/detectors/UniswapPublicCallback.md
+[public_hook]: docs/detectors/UniswapPublicHook.md
+[upgradable_hook]: docs/detectors/UniswapUpgradableHook.md
+[suicidal_hook]: docs/detectors/UniswapSuicidalHook.md
diff --git a/docs/detectors/UniswapPublicCallback.md b/docs/detectors/UniswapPublicCallback.md
@@ -0,0 +1,40 @@
+# UniswapPublicCallback
+
+## Info
+
+**Spec**
+
+- Severity: High
+- Confidence: High
+
+**Description**
+
+Callers of callback functions are not exclusively restricted to the contract itself.
+
+## Sample
+
+```diff
+abstract contract BaseHook is IHooks {
+    modifier selfOnly() {
+        if (msg.sender != address(this)) revert NotSelf();
+        _;
+    }
+}
+
+contract Hook is BaseHook {
+    uint count;
+
+    constructor(IPoolManager _poolManager) BaseHook(_poolManager) {}
+
+    function foo() external {
+        poolManager.lock(abi.encodeWithSignature("callback()"));
+    }
+
+-    function callback() external {
++    function callback() external selfOnly {
+        count++;
+    }
+}
+```
+
+For any callback functions that are called by lockAcquired using external calls, there should be an only-self check (no need for internal callback).
diff --git a/docs/detectors/UniswapPublicHook.md b/docs/detectors/UniswapPublicHook.md
@@ -0,0 +1,48 @@
+# UniswapPublicHook
+
+## Info
+
+**Spec**
+
+- Severity: High
+- Confidence: High
+
+**Description**
+
+Callers of hook functions are not exclusively restricted to the pool manager alone.
+
+## Sample
+
+```diff
+abstract contract BaseHook is IHooks {
+    IPoolManager public immutable poolManager;
+
+    constructor(IPoolManager _poolManager) {
+        poolManager = _poolManager;
+    }
+
+    modifier poolManagerOnly() {
+        if (msg.sender != address(poolManager)) revert NotPoolManager();
+        _;
+    }
+}
+
+contract Hook is BaseHook {
+    uint count;
+
+    constructor(IPoolManager _poolManager) BaseHook(_poolManager) {}
+
+    function beforeSwap(
+        address,
+        PoolKey calldata,
+        IPoolManager.SwapParams calldata,
+        bytes calldata
+-    ) external override returns (bytes4) {
++    ) external override poolManagerOnly returns (bytes4) {
+        count++;  // make changes to contract states
+        return IHooks.beforeSwap.selector;
+    }
+}
+```
+
+This detector enumerates all the hook functions (e.g. `beforeSwap`) that are not `view` (i.e., read only) and can be called by anyone without privilege validation.
diff --git a/docs/detectors/UniswapSuicidalHook.md b/docs/detectors/UniswapSuicidalHook.md
@@ -0,0 +1,13 @@
+# UniswapSuicidalHook
+
+## Info
+
+**Spec**
+
+- Severity: Medium
+- Confidence: High
+
+**Description**
+
+The contract contains `SELFDESTRUCT`.
+No self-destruct is allowed, even with privilege validation.
diff --git a/docs/detectors/UniswapUpgradableHook.md b/docs/detectors/UniswapUpgradableHook.md
@@ -0,0 +1,12 @@
+# UniswapUpgradableHook
+
+## Info
+
+**Spec**
+
+- Severity: High
+- Confidence: High
+
+**Description**
+
+The contract `DELEGATECALL`s to mutable addresses.