ChainSec audit

chronicleprotocol · Jul 24, 2023 · d2534c1 · d2534c1
1 parent e4f1f66
commit d2534c1
Show file tree

Hide file tree

Showing 13 changed files with 250 additions and 186 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
@@ -1,24 +1,22 @@
-AggorTest:test_Deployment() (gas: 22277)
-AggorTest:test_latestAnswer_IsTollProtected() (gas: 11829)
-AggorTest:test_latestRoundData_IsTollProtected() (gas: 12872)
-AggorTest:test_poke_ChainlinkDecimalConversion() (gas: 181275)
-AggorTest:test_readWithAge_FailsIfValIsZero() (gas: 13444)
-AggorTest:test_readWithAge_IsTollProtected() (gas: 11621)
-AggorTest:test_read_FailsIfValIsZero() (gas: 12873)
-AggorTest:test_read_IsTollProtected() (gas: 12377)
-AggorTest:test_setSpread_IsAuthProtected() (gas: 12118)
-AggorTest:test_setStalenessThreshold_FailsIf_IsZero() (gas: 11348)
-AggorTest:test_setStalenessThreshold_IsAuthProtected() (gas: 11901)
-AggorTest:test_setUniSecondsAgo_IsAuthProtected() (gas: 12253)
-AggorTest:test_setUniswap_FromZeroAddress() (gas: 91741)
-AggorTest:test_setUniswap_IsAuthProtected() (gas: 12222)
-AggorTest:test_setUniswap_ToZeroAddress() (gas: 69117)
-AggorTest:test_tryReadWithAge_IsTollProtected() (gas: 12790)
-AggorTest:test_tryReadWithAge_ReturnsFalseIfValIsZero() (gas: 11181)
-AggorTest:test_tryRead_IsTollProtected() (gas: 11984)
-AggorTest:test_tryRead_ReturnsFalseIfValIsZero() (gas: 10068)
+AggorTest:test_Deployment() (gas: 20366)
+AggorTest:test_latestAnswer_IsTollProtected() (gas: 11841)
+AggorTest:test_latestRoundData_IsTollProtected() (gas: 12839)
+AggorTest:test_poke_ChainlinkDecimalConversion() (gas: 156430)
+AggorTest:test_readWithAge_FailsIfValIsZero() (gas: 13424)
+AggorTest:test_readWithAge_IsTollProtected() (gas: 11614)
+AggorTest:test_read_FailsIfValIsZero() (gas: 12893)
+AggorTest:test_read_IsTollProtected() (gas: 12388)
+AggorTest:test_setSpread_IsAuthProtected() (gas: 12109)
+AggorTest:test_setStalenessThreshold_FailsIf_IsZero() (gas: 11320)
+AggorTest:test_setStalenessThreshold_IsAuthProtected() (gas: 11889)
+AggorTest:test_setUniSecondsAgo_IsAuthProtected() (gas: 12244)
+AggorTest:test_tryReadWithAge_IsTollProtected() (gas: 12777)
+AggorTest:test_tryReadWithAge_ReturnsFalseIfValIsZero() (gas: 11157)
+AggorTest:test_tryRead_IsTollProtected() (gas: 11952)
+AggorTest:test_tryRead_ReturnsFalseIfValIsZero() (gas: 10070)
+AggorTest:test_useUniswap_IsAuthProtected() (gas: 12326)
+AggorTest:test_useUniswap_NotConfigured() (gas: 2345964)
 LibCalcTest:test_distance() (gas: 305)
 LibCalcTest:test_pctDiff() (gas: 439)
 LibCalcTest:test_scale_basic() (gas: 240)
-LibCalcTest:test_scale_revert() (gas: 3398)
-MainnetIntegrationTest:testIntegration_Mainnet() (gas: 214755)
+LibCalcTest:test_scale_revert() (gas: 3398)
diff --git a/.github/workflows/gas.yml b/.github/workflows/gas.yml
@@ -22,4 +22,4 @@ jobs:
 
       - name: Check gas snapshots
         run: |
-          forge snapshot --nmt "Fuzz|Mainnet" --check
+          forge snapshot --nmt "Fuzz|Integration" --check
diff --git a/Makefile b/Makefile
@@ -10,7 +10,7 @@ gas_report: ## Print gas report
 
 .PHONY: snapshot
 snapshot: ## Update forge's snapshot file
-	forge snapshot --nmt "Fuzz"
+	forge snapshot --nmt "Fuzz|Integration"
 
 .PHONY: help
 help: ## Help command

diff --git a/docs/Aggor.md b/docs/Aggor.md
@@ -35,36 +35,50 @@ The following `IChainlinkAggregatorV3` functions are provided:
 - `latestAnswer()`
 - `decimals()`
 
+## Low liquidity Uniswap pools
+
+If a low liquidity pool is used in Aggor, the cost of price manipulation of the internal uniswap oracle is drastically reduced. However, because of the technicalities required, an attacker will at least need to control two consecutive blocks (potentially more) to make their manipulation succesful, which mitigates the risk. High liquidity pools will always be sought first as well, and deployments will only be done after a thorough risk analysis.
+
+## Price jump
+
+The spread limit guards against single outliers in one of the price feeds, making the aggregate price generally less volatile than the sources. However, if the values of the two feeds diverge slowly, the price will experience a sudden jump of approximately `spread/2` when the difference is greater than `spread`.
+
+## Price manipulation
+
+There is an inherent trust assumption in Aggor that the oracle *sources* are difficult to manipulate, that is, designed in and of themselves to be manipulation resistant, especially for a significant amount of time.
+
+Aggor’s `spread` minimizes this risk further by preventing large deviations in a single update. Effectively, customers do not need to trust a given oracle system at any point in time. However, over some period of time trust in both oracles is required.
+
+For example, if one oracle ceases to update, but the other does not, Aggor price will (in time) "go stale" and give the price that was last agreed upon when both oracle systems were reporting. This timeframe should allow enough time for an operational reaction. Good monitoring and contingencies will be necessary, and in place to further guard against price manipulation.
+
 ## Benchmarks
 
 A gas report for the `poke()` function can be created via `make gas_report`.
 
-```bash
-$ make gas_report
-
-Gas report taken Wed Jun  7 10:45:35 UTC 2023
+```
+Gas report taken Thu Jul 20 18:23:19 UTC 2023
 forge t --gas-report --match-test poke_basic
 [⠒] Compiling...
 No files changed, compilation skipped
 
 Running 1 test for test/Runner.t.sol:AggorTest
-[PASS] testFuzz_poke_basic(uint128,uint128,uint256,uint256,uint256) (runs: 256, μ: 167561, ~: 167689)
-Test result: ok. 1 passed; 0 failed; finished in 49.07ms
+[PASS] testFuzz_poke_basic(uint128,uint128,uint256,uint256,uint256) (runs: 256, μ: 165830, ~: 165956)
+Test result: ok. 1 passed; 0 failed; finished in 46.77ms
 | src/Aggor.sol:Aggor contract |                 |       |        |       |         |
 |------------------------------|-----------------|-------|--------|-------|---------|
 | Deployment Cost              | Deployment Size |       |        |       |         |
-| 2334115                      | 12122           |       |        |       |         |
+| 2200182                      | 12072           |       |        |       |         |
 | Function Name                | min             | avg   | median | max   | # calls |
-| chainlink                    | 678             | 678   | 678    | 678   | 1       |
-| chronicle                    | 722             | 722   | 722    | 722   | 1       |
-| kiss                         | 69382           | 69382 | 69382  | 69382 | 1       |
-| latestAnswer                 | 769             | 769   | 769    | 769   | 1       |
-| latestRoundData              | 1283            | 1283  | 1283   | 1283  | 1       |
-| poke                         | 33036           | 33036 | 33036  | 33036 | 1       |
-| read                         | 810             | 810   | 810    | 810   | 1       |
-| readWithAge                  | 707             | 707   | 707    | 707   | 1       |
-| spread                       | 630             | 630   | 630    | 630   | 1       |
-| stalenessThreshold           | 856             | 1856  | 1856   | 2856  | 2       |
-| tryRead                      | 525             | 1525  | 1525   | 2525  | 2       |
-| tryReadWithAge               | 1181            | 1181  | 1181   | 1181  | 1       |
+| chainlink                    | 700             | 700   | 700    | 700   | 1       |
+| chronicle                    | 744             | 744   | 744    | 744   | 1       |
+| kiss                         | 69377           | 69377 | 69377  | 69377 | 1       |
+| latestAnswer                 | 819             | 819   | 819    | 819   | 1       |
+| latestRoundData              | 1311            | 1311  | 1311   | 1311  | 1       |
+| poke                         | 31005           | 31005 | 31005  | 31005 | 1       |
+| read                         | 860             | 860   | 860    | 860   | 1       |
+| readWithAge                  | 735             | 735   | 735    | 735   | 1       |
+| spread                       | 674             | 674   | 674    | 674   | 1       |
+| stalenessThreshold           | 878             | 1878  | 1878   | 2878  | 2       |
+| tryRead                      | 553             | 1553  | 1553   | 2553  | 2       |
+| tryReadWithAge               | 1209            | 1209  | 1209   | 1209  | 1       |
 ```
diff --git a/script/Aggor.s.sol b/script/Aggor.s.sol
@@ -25,16 +25,24 @@ import {Aggor} from "src/Aggor.sol";
  */
 contract AggorScript is Script {
     /// @dev You'll want to adjust this addresses before deployment.
-    ///      Note that deployment fails if addresses are zero.
+    ///      Note that deployment fails if addresses are zero (with the
+    //       exception of Uniswap, which is optional).
     address internal constant ORACLE_CHRONICLE = address(0);
     address internal constant ORACLE_CHAINLINK = address(0);
+    address internal constant ORACLE_UNISWAP = address(0);
+    bool internal constant UNI_USE_TOKEN0_AS_BASE = false;
 
     /// @dev You'll want to adjust this address if Aggor is already deployed.
     IAggor internal aggor = IAggor(address(0));
 
     function deploy() public returns (IAggor) {
         vm.startBroadcast();
-        aggor = new Aggor(ORACLE_CHRONICLE, ORACLE_CHAINLINK);
+        aggor = new Aggor(
+            ORACLE_CHRONICLE,
+            ORACLE_CHAINLINK,
+            ORACLE_UNISWAP,
+            UNI_USE_TOKEN0_AS_BASE
+        );
         vm.stopBroadcast();
 
         return aggor;