🔒 Amend Security Note in Multicall Contract (#193)

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
pcaversaccio · Dec 19, 2023 · 2294f90 · 2294f90
1 parent ad6a2ba
commit 2294f90
Show file tree

Hide file tree

Showing 7 changed files with 124 additions and 113 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml
@@ -111,4 +111,4 @@ jobs:
           test/**/*.sol test/**/interfaces/*.sol test/**/mocks/*.sol \
           test/**/scripts/*.js scripts/*.py --allow-dupe --allow-redirect \
           --request-delay 0.4 \
-          --white-list https://www.wagmi.xyz,https://twitter.com/0xDACA/status/1669846430528286722,https://github.com/pcaversaccio/snekmate/releases/tag/v0.0.5,https://github.com/pcaversaccio/snekmate/blob/v0.0.5,https://github.com/pcaversaccio/snekmate/compare/v0.0.4...v0.0.5
+          --white-list https://www.wagmi.xyz,https://twitter.com,https://github.com/pcaversaccio/snekmate/releases/tag/v0.0.5,https://github.com/pcaversaccio/snekmate/blob/v0.0.5,https://github.com/pcaversaccio/snekmate/compare/v0.0.4...v0.0.5
diff --git a/lib/create-util b/lib/create-util
diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts
diff --git a/lib/solady b/lib/solady
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/src/utils/Multicall.vy b/src/utils/Multicall.vy
@@ -12,9 +12,15 @@
         https://github.com/pcaversaccio/snekmate/discussions/82.
         The implementation is inspired by Matt Solomon's implementation here:
         https://github.com/mds1/multicall/blob/main/src/Multicall3.sol.
-@custom:security Make sure you understand how `msg.sender` works in `CALL` vs
-                 `DELEGATECALL` to the multicall contract, as well as the risks
-                 of using `msg.value` in a multicall. To learn more about the latter, see:
+@custom:security You must ensure that any contract that integrates the `CALL`-based
+                 `multicall` and `multicall_value` functions never holds funds after
+                 the end of a transaction. Otherwise, any ETH, tokens, or other funds
+                 held by this contract can be stolen. Also, never approve a contract
+                 that integrates the `CALL`-based functions `multicall` and `multicall_value`
+                 to spend your tokens. If you do, anyone can steal your tokens! Eventually,
+                 please make sure you understand how `msg.sender` works in `CALL` vs
+                 `DELEGATECALL` to the multicall contract, as well as the risks of
+                 using `msg.value` in a multicall. To learn more about the latter, see:
                  - https://github.com/runtimeverification/verified-smart-contracts/wiki/List-of-Security-Vulnerabilities#payable-multicall,
                  - https://samczsun.com/two-rights-might-make-a-wrong.
 """
@@ -108,6 +114,11 @@ def multicall_value(data: DynArray[BatchValue, max_value(uint8)]) -> DynArray[Re
     success: bool = empty(bool)
     for batch in data:
         msg_value: uint256 = batch.value
+        # WARNING: If you expect to hold any funds in a contract that integrates
+        # this function, you must ensure that the next line uses checked arithmetic!
+        # Please read the contract-level security notice carefully. For further
+        # insights also, see the following Twitter thread:
+        # https://twitter.com/Guhu95/status/1736983530343981307.
         value_accumulator = unsafe_add(value_accumulator, msg_value)
         if (batch.allow_failure == False):
             return_data = raw_call(batch.target, batch.call_data, max_outsize=255, value=msg_value)
+1 −1		.github/workflows/checks.yml
+2 −2		.github/workflows/test-contracts.yml
+7 −11		hardhat.config.ts
+7 −8		package.json
+173 −173		pnpm-lock.yaml
+12 −5		scripts/deploy.ts
+2 −18		package-lock.json
+0 −1		package.json
+489 −505		test/governance/Governor.test.js
+824 −829		test/governance/TimelockController.test.js
+98 −83		test/governance/extensions/GovernorERC721.test.js
+118 −128		test/governance/extensions/GovernorPreventLateQuorum.test.js
+107 −102		test/governance/extensions/GovernorStorage.test.js
+415 −443		test/governance/extensions/GovernorTimelockAccess.test.js
+227 −220		test/governance/extensions/GovernorTimelockCompound.test.js
+254 −265		test/governance/extensions/GovernorTimelockControl.test.js
+81 −83		test/governance/extensions/GovernorVotesQuorumFraction.test.js
+151 −176		test/governance/extensions/GovernorWithParams.test.js
+5 −5		test/governance/utils/ERC6372.behavior.js
+219 −248		test/governance/utils/Votes.behavior.js
+60 −50		test/governance/utils/Votes.test.js
+142 −197		test/helpers/governance.js
+7 −0		test/helpers/iterate.js
+10 −1		test/helpers/methods.js
+5 −6		test/helpers/time.js
+7 −18		test/helpers/txpool.js
+2 −4		test/token/ERC20/extensions/ERC20Permit.test.js
+385 −426		test/token/ERC20/extensions/ERC20Votes.test.js
+98 −108		test/token/ERC20/utils/SafeERC20.test.js
+136 −125		test/token/ERC721/extensions/ERC721Votes.test.js
+9 −2		test/utils/introspection/ERC165.test.js
+82 −137		test/utils/introspection/ERC165Checker.test.js
+10 −16		test/utils/introspection/SupportsInterface.behavior.js
+10 −9		.gas-snapshot
+7 −7		js/solady.js
+1 −1		package.json
+26 −0		src/utils/LibPRNG.sol
+33 −0		test/LibPRNG.t.sol