📝 Add 0xth0mas review (#29)

Vectorized · Dec 14, 2023 · b4a0dd0 · b4a0dd0
1 parent 8c07107
commit b4a0dd0
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 8 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
@@ -1,12 +1,12 @@
-MulticallerTest:testMultiCallerWithSignerIncrementNonceSalt(uint256) (runs: 256, μ: 83521, ~: 82301)
-MulticallerTest:testMultiCallerWithSignerIncrementNonceSaltWithERC1271(uint256) (runs: 256, μ: 614878, ~: 614930)
+MulticallerTest:testMultiCallerWithSignerIncrementNonceSalt(uint256) (runs: 256, μ: 83320, ~: 82376)
+MulticallerTest:testMultiCallerWithSignerIncrementNonceSaltWithERC1271(uint256) (runs: 256, μ: 614873, ~: 614814)
 MulticallerTest:testMulticallerCdFallback(string) (runs: 256, μ: 295907, ~: 281868)
 MulticallerTest:testMulticallerForwardsMessageValue() (gas: 214013)
 MulticallerTest:testMulticallerGetNames() (gas: 147637)
 MulticallerTest:testMulticallerReentrancyGuard() (gas: 19980)
-MulticallerTest:testMulticallerRefund(uint256) (runs: 256, μ: 169978, ~: 172012)
+MulticallerTest:testMulticallerRefund(uint256) (runs: 256, μ: 170012, ~: 172026)
 MulticallerTest:testMulticallerReturnDataIsProperlyEncoded() (gas: 122173)
-MulticallerTest:testMulticallerReturnDataIsProperlyEncoded(string,string,uint256) (runs: 256, μ: 487938, ~: 508582)
+MulticallerTest:testMulticallerReturnDataIsProperlyEncoded(string,string,uint256) (runs: 256, μ: 506602, ~: 531965)
 MulticallerTest:testMulticallerReturnDataIsProperlyEncoded(uint256,uint256,uint256,uint256) (runs: 256, μ: 122252, ~: 122252)
 MulticallerTest:testMulticallerRevertWithCustomError() (gas: 35485)
 MulticallerTest:testMulticallerRevertWithMessage() (gas: 38244)
@@ -15,15 +15,15 @@ MulticallerTest:testMulticallerRevertWithNothing() (gas: 35306)
 MulticallerTest:testMulticallerSenderDoesNotRevertWithoutMulticallerDeployed() (gas: 3423)
 MulticallerTest:testMulticallerTargetGetMulticallerSender() (gas: 27448)
 MulticallerTest:testMulticallerWithNoData() (gas: 16213)
-MulticallerTest:testMulticallerWithSigner(uint256) (runs: 256, μ: 131230, ~: 119355)
+MulticallerTest:testMulticallerWithSigner(uint256) (runs: 256, μ: 130354, ~: 119358)
 MulticallerTest:testMulticallerWithSignerEIP712Domain() (gas: 12375)
 MulticallerTest:testMulticallerWithSignerGetMulticallerSigner() (gas: 136389)
-MulticallerTest:testMulticallerWithSignerInvalidateNonces(uint256) (runs: 256, μ: 81419, ~: 79804)
-MulticallerTest:testMulticallerWithSignerInvalidateNoncesWithERC1271(uint256) (runs: 256, μ: 616664, ~: 616391)
+MulticallerTest:testMulticallerWithSignerInvalidateNonces(uint256) (runs: 256, μ: 80313, ~: 79643)
+MulticallerTest:testMulticallerWithSignerInvalidateNoncesWithERC1271(uint256) (runs: 256, μ: 616631, ~: 616541)
 MulticallerTest:testMulticallerWithSignerNonPayableFunctions() (gas: 48884)
 MulticallerTest:testMulticallerWithSignerReentrancyGuard() (gas: 124909)
 MulticallerTest:testMulticallerWithSignerRevert() (gas: 206240)
-MulticallerTest:testMulticallerWithSignerWithERC1271(uint256) (runs: 256, μ: 659790, ~: 651359)
+MulticallerTest:testMulticallerWithSignerWithERC1271(uint256) (runs: 256, μ: 657836, ~: 651346)
 MulticallerTest:testMulticallerWithSignerWithNoData() (gas: 130147)
 MulticallerTest:testNastyCalldataRevert() (gas: 3420)
 MulticallerTest:testOffsetTrick(uint256,uint256,uint256) (runs: 256, μ: 571, ~: 571)

diff --git a/audits/0xth0mas_review.md b/audits/0xth0mas_review.md
@@ -0,0 +1,65 @@
+Original at: https://gist.github.com/0xth0mas/59ea1f31f16e058da27b0607c4dc24b8
+
+# General
+
+No major vulnerabilities were identified.
+
+# Scope
+
+Review for [Vectorized/multicaller commit 8c07107](https://github.com/Vectorized/multicaller/commit/8c071078b29d0037a7f01ec6f346776ec7c89948)
+
+```
+src
+├── LibMulticaller.sol
+└── MulticallerWithSigner.sol
+```
+
+---
+
+### Documentation for reentrancy guard bit references incorrect bit location
+
+**Severity:** Informational
+**Likelihood:** N/A
+**File:** [MulticallerWithSigner](https://github.com/Vectorized/multicaller/blob/8c071078b29d0037a7f01ec6f346776ec7c89948/src/MulticallerWithSigner.sol#L122)
+
+The comment in the constructor detailing the data packing of storage slot 0 references `bit 1` as the location for the `MulticallerWithSigner` reentrancy guard while it is stored at bit 0.
+
+This has no impact to the functionality of `MulticallerWithSigner`. The only potential negative impact would be from a developer forking `MulticallerWithSigner` without understanding the code and making assumptions about the reentrancy guard based on the comment.
+
+**Mitigation:**
+
+Update comment to reflect the reentrancy guard at bit 0.
+
+---
+
+### Potential gas optimization on `resultsOffset` incrementer
+
+**Severity:** Gas Optimization
+**Likelihood:** Unknown
+**File:** [MulticallerWithSigner](https://github.com/Vectorized/multicaller/blob/8c071078b29d0037a7f01ec6f346776ec7c89948/src/MulticallerWithSigner.sol#L319)
+
+Incrementing the `resultsOffset` value with the necessary padding for ABI encoding of results values requires eight operations that are unnecessary for the final iteration of the loop.
+
+It may be more efficient to move the `resultsOffset` incrementer after the loop end check. Mock tests show a gas savings of 24 gas units for a single call with each additional call in the multicall transaction reducing savings by 6 gas units until it hits a breakeven point at 5 calls and becomes more expensive at 6 calls. 
+
+If the average number of calls included in a multicall transaction to `MulticallerWithSigner` is under 5 it would be beneficial to change the code as follows:
+
+##### From - 
+```solidity
+                // Advance the `resultsOffset` by `returndatasize() + 0x20`,
+                // rounded up to the next multiple of 0x20.
+                resultsOffset := and(add(add(resultsOffset, returndatasize()), 0x3f), not(0x1f))
+                // Advance the `results` pointer.
+                results := add(results, 0x20)
+                if eq(results, end) { break }
+```
+
+#### To -
+```solidity
+                // Advance the `results` pointer.
+                results := add(results, 0x20)
+                if eq(results, end) { break }
+                // Advance the `resultsOffset` by `returndatasize() + 0x20`,
+                // rounded up to the next multiple of 0x20.
+                resultsOffset := and(add(add(resultsOffset, returndatasize()), 0x3f), not(0x1f))
+```