Address chainsecurity #1 comment (#114)

* address chainsecurity #1 comment

* Update test/CodeJar.t.sol

Co-authored-by: Kevin Cheng <[email protected]>

* assert codehash is not 0

* update snapshot

---------

Co-authored-by: Kevin Cheng <[email protected]>

.gas-snapshot

@@ -1,59 +1,60 @@
-BatchExecutorTest:testBatchExecute() (gas: 132910)
-BatchExecutorTest:testBatchExecuteRevertsIfAnyCallReverts() (gas: 123542)
+BatchExecutorTest:testBatchExecute() (gas: 132880)
+BatchExecutorTest:testBatchExecuteRevertsIfAnyCallReverts() (gas: 123504)
 BatchExecutorTest:testBatchExecuteRevertsOnBadInput() (gas: 47594)
 CallbacksTest:testAllowNestedCallbacks() (gas: 148651)
-CallbacksTest:testCallbackFromCounter() (gas: 105646)
+CallbacksTest:testCallbackFromCounter() (gas: 105608)
 CallbacksTest:testCallcodeReentrancyExploit() (gas: 104988)
 CallbacksTest:testCallcodeReentrancyProtection() (gas: 187608)
 CallbacksTest:testNestedCallWithNoCallbackSucceeds() (gas: 116480)
-CallbacksTest:testPayableCallback() (gas: 113170)
-CallbacksTest:testRevertsOnCallbackWhenNoActiveCallback() (gas: 99393)
-CodeJarTest:testCodeJarCounter() (gas: 376967)
-CodeJarTest:testCodeJarDeployConstructor() (gas: 46721)
-CodeJarTest:testCodeJarDifferentZeros() (gas: 89485)
-CodeJarTest:testCodeJarFirstDeploy() (gas: 44400)
-CodeJarTest:testCodeJarInputVariety() (gas: 449301)
-CodeJarTest:testCodeJarLarge() (gas: 69578633)
-CodeJarTest:testCodeJarSecondDeploy() (gas: 47327)
-CodeJarTest:testCodeJarSelfDestruct() (gas: 50682)
-CometClaimRewardsTest:testClaimComp() (gas: 131614)
-CometRepayAndWithdrawMultipleAssetsTest:testInvalidInput() (gas: 77279)
-CometRepayAndWithdrawMultipleAssetsTest:testRepayAndWithdrawMultipleAssets() (gas: 161195)
-CometSupplyMultipleAssetsAndBorrowTest:testInvalidInput() (gas: 77436)
-CometSupplyMultipleAssetsAndBorrowTest:testSupplyMultipleAssetsAndBorrow() (gas: 303015)
+CallbacksTest:testPayableCallback() (gas: 113132)
+CallbacksTest:testRevertsOnCallbackWhenNoActiveCallback() (gas: 99355)
+CodeJarTest:testCodeJarCounter() (gas: 377506)
+CodeJarTest:testCodeJarDeployConstructor() (gas: 47037)
+CodeJarTest:testCodeJarDeployNotAffectedByChangedCodeHash() (gas: 81705)
+CodeJarTest:testCodeJarDifferentZeros() (gas: 90061)
+CodeJarTest:testCodeJarFirstDeploy() (gas: 44694)
+CodeJarTest:testCodeJarInputVariety() (gas: 451689)
+CodeJarTest:testCodeJarLarge() (gas: 70676086)
+CodeJarTest:testCodeJarSecondDeploy() (gas: 47605)
+CodeJarTest:testCodeJarSelfDestruct() (gas: 50952)
+CometClaimRewardsTest:testClaimComp() (gas: 131576)
+CometRepayAndWithdrawMultipleAssetsTest:testInvalidInput() (gas: 77241)
+CometRepayAndWithdrawMultipleAssetsTest:testRepayAndWithdrawMultipleAssets() (gas: 161164)
+CometSupplyMultipleAssetsAndBorrowTest:testInvalidInput() (gas: 77398)
+CometSupplyMultipleAssetsAndBorrowTest:testSupplyMultipleAssetsAndBorrow() (gas: 302984)
 ConditionalMulticallTest:testConditionalRunEmptyInputIsValid() (gas: 49056)
 ConditionalMulticallTest:testConditionalRunInvalidInput() (gas: 65592)
 ConditionalMulticallTest:testConditionalRunMulticallError() (gas: 312861)
 ConditionalMulticallTest:testConditionalRunOnPeriodicRepay() (gas: 327133)
 ConditionalMulticallTest:testConditionalRunPassed() (gas: 281293)
 ConditionalMulticallTest:testConditionalRunUnmet() (gas: 100273)
-EIP1271Test:testReturnsMagicValueForValidSignature() (gas: 79893)
+EIP1271Test:testReturnsMagicValueForValidSignature() (gas: 79855)
 EIP1271Test:testRevertsIfSignerContractDoesNotReturnMagic() (gas: 20524)
-EIP712Test:testExecuteQuarkOperation() (gas: 79333)
-EIP712Test:testNonceIsNotSetForReplayableOperation() (gas: 152052)
-EIP712Test:testRequirements() (gas: 194448)
+EIP712Test:testExecuteQuarkOperation() (gas: 79295)
+EIP712Test:testNonceIsNotSetForReplayableOperation() (gas: 151991)
+EIP712Test:testRequirements() (gas: 194334)
 EIP712Test:testRevertBadRequirements() (gas: 29986)
 EIP712Test:testRevertsForBadCalldata() (gas: 23259)
 EIP712Test:testRevertsForBadCode() (gas: 21560)
 EIP712Test:testRevertsForBadExpiry() (gas: 23778)
 EIP712Test:testRevertsForExpiredSignature() (gas: 14266)
 EIP712Test:testRevertsInvalidS() (gas: 20356)
-EIP712Test:testRevertsOnReusedNonce() (gas: 103380)
-EthcallTest:testEthcallCallReraiseError() (gas: 78556)
-EthcallTest:testEthcallCounter() (gas: 72055)
-EthcallTest:testEthcallShouldReturnCallResult() (gas: 54491)
-EthcallTest:testEthcallSupplyUSDCToComet() (gas: 175859)
-EthcallTest:testEthcallWithdrawUSDCFromComet() (gas: 301904)
+EIP712Test:testRevertsOnReusedNonce() (gas: 103304)
+EthcallTest:testEthcallCallReraiseError() (gas: 78518)
+EthcallTest:testEthcallCounter() (gas: 72017)
+EthcallTest:testEthcallShouldReturnCallResult() (gas: 54460)
+EthcallTest:testEthcallSupplyUSDCToComet() (gas: 175798)
+EthcallTest:testEthcallWithdrawUSDCFromComet() (gas: 301812)
 ExecutorTest:testExecutorCanDirectCall() (gas: 126176)
-ExecutorTest:testExecutorCanDirectCallBySig() (gas: 118532)
+ExecutorTest:testExecutorCanDirectCallBySig() (gas: 118501)
 MulticallTest:testCreateSubWalletAndExecute() (gas: 1293506)
-MulticallTest:testEmptyInputIsValid() (gas: 55368)
-MulticallTest:testExecutorCanMulticallAcrossSubwallets() (gas: 298675)
-MulticallTest:testInvalidInput() (gas: 73840)
-MulticallTest:testInvokeCounterTwice() (gas: 88802)
-MulticallTest:testMulticallError() (gas: 314933)
-MulticallTest:testMulticallShouldReturnCallResults() (gas: 92019)
-MulticallTest:testSupplyWETHWithdrawUSDCOnComet() (gas: 265033)
+MulticallTest:testEmptyInputIsValid() (gas: 55337)
+MulticallTest:testExecutorCanMulticallAcrossSubwallets() (gas: 298644)
+MulticallTest:testInvalidInput() (gas: 73802)
+MulticallTest:testInvokeCounterTwice() (gas: 88764)
+MulticallTest:testMulticallError() (gas: 314895)
+MulticallTest:testMulticallShouldReturnCallResults() (gas: 91981)
+MulticallTest:testSupplyWETHWithdrawUSDCOnComet() (gas: 264995)
 NonceTest:testIsSet() (gas: 30425)
 NonceTest:testNextUnusedNonce() (gas: 41215)
 NonceTest:testNonLinearNonce() (gas: 30757)
@@ -63,31 +64,31 @@ QuarkStateManagerTest:testScriptAddressIsEOA() (gas: 45228)
 QuarkStateManagerTest:testScriptAddressIsNull() (gas: 41725)
 QuarkStateManagerTest:testSetActiveNonceAndCallbackNotImplemented() (gas: 92016)
 QuarkWalletFactoryTest:testCreateAdditionalWalletWithSalt() (gas: 8937393460517580219)
-QuarkWalletFactoryTest:testCreateAndExecuteCreatesWallet() (gas: 1127206)
-QuarkWalletFactoryTest:testCreateAndExecuteSetsMsgSender() (gas: 919038)
-QuarkWalletFactoryTest:testCreateAndExecuteWithSalt() (gas: 1194522)
-QuarkWalletFactoryTest:testCreateAndExecuteWithSaltSetsMsgSender() (gas: 957299)
+QuarkWalletFactoryTest:testCreateAndExecuteCreatesWallet() (gas: 1127833)
+QuarkWalletFactoryTest:testCreateAndExecuteSetsMsgSender() (gas: 919369)
+QuarkWalletFactoryTest:testCreateAndExecuteWithSalt() (gas: 1195149)
+QuarkWalletFactoryTest:testCreateAndExecuteWithSaltSetsMsgSender() (gas: 957630)
 QuarkWalletFactoryTest:testCreateRevertsOnRepeat() (gas: 8937393460516754977)
 QuarkWalletFactoryTest:testCreatesCodejar() (gas: 5986)
 QuarkWalletFactoryTest:testCreatesWalletAtDeterministicAddress() (gas: 1691686)
 QuarkWalletFactoryTest:testDefaultWalletHasNoExecutor() (gas: 794985)
-QuarkWalletFactoryTest:testDefaultWalletIsSubwalletExecutor() (gas: 275015)
-QuarkWalletFactoryTest:testExecuteOnExistingWallet() (gas: 1125776)
+QuarkWalletFactoryTest:testDefaultWalletIsSubwalletExecutor() (gas: 275554)
+QuarkWalletFactoryTest:testExecuteOnExistingWallet() (gas: 1126403)
 QuarkWalletFactoryTest:testVersion() (gas: 6047)
-QuarkWalletTest:testAllowAllNullNoopScript() (gas: 109730)
+QuarkWalletTest:testAllowAllNullNoopScript() (gas: 109956)
 QuarkWalletTest:testAtomicIncrementerWithScriptAddress() (gas: 68053)
-QuarkWalletTest:testAtomicIncrementerWithScriptSource() (gas: 75733)
+QuarkWalletTest:testAtomicIncrementerWithScriptSource() (gas: 75695)
 QuarkWalletTest:testAtomicMaxCounterScriptWithScriptAddress() (gas: 257644)
-QuarkWalletTest:testAtomicMaxCounterScriptWithScriptSource() (gas: 287552)
+QuarkWalletTest:testAtomicMaxCounterScriptWithScriptSource() (gas: 287431)
 QuarkWalletTest:testAtomicPingWithScriptAddress() (gas: 49151)
-QuarkWalletTest:testAtomicPingWithScriptSource() (gas: 52050)
-QuarkWalletTest:testCanReplaySameScriptWithDifferentCall() (gas: 155395)
+QuarkWalletTest:testAtomicPingWithScriptSource() (gas: 52020)
+QuarkWalletTest:testCanReplaySameScriptWithDifferentCall() (gas: 155364)
 QuarkWalletTest:testGetCodeJar() (gas: 8540)
 QuarkWalletTest:testGetExecutor() (gas: 5629)
 QuarkWalletTest:testGetSigner() (gas: 8636)
 QuarkWalletTest:testGetStateManager() (gas: 7832)
 QuarkWalletTest:testNoopScriptIsValidForScriptAddress() (gas: 49509)
-QuarkWalletTest:testNoopScriptIsValidForScriptSource() (gas: 52014)
+QuarkWalletTest:testNoopScriptIsValidForScriptSource() (gas: 51984)
 QuarkWalletTest:testPrecompileBigModExp() (gas: 48282)
 QuarkWalletTest:testPrecompileBigModExpWithoutScript() (gas: 47887)
 QuarkWalletTest:testPrecompileBlake2F() (gas: 50607)
@@ -105,49 +106,49 @@ QuarkWalletTest:testPrecompileRipemd160WithoutScript() (gas: 48769)
 QuarkWalletTest:testPrecompileSha256() (gas: 50223)
 QuarkWalletTest:testPrecompileSha256WithoutScript() (gas: 48085)
 QuarkWalletTest:testQuarkOperationWithScriptAddressRevertsIfCallReverts() (gas: 64052)
-QuarkWalletTest:testQuarkOperationWithScriptSourceRevertsIfCallReverts() (gas: 72028)
+QuarkWalletTest:testQuarkOperationWithScriptSourceRevertsIfCallReverts() (gas: 71990)
 QuarkWalletTest:testRevertsForDirectExecuteByNonExecutorSigner() (gas: 9994)
 QuarkWalletTest:testRevertsForOperationWithAddressAndSource() (gas: 11402)
 QuarkWalletTest:testRevertsForReplayOfCanceledScript() (gas: 198213)
-QuarkWalletTest:testRevertsForReusedNonceWithChangedScript() (gas: 149044)
+QuarkWalletTest:testRevertsForReusedNonceWithChangedScript() (gas: 149326)
 QuarkWalletTest:testRevertsForUnauthorizedDirectExecuteByRandomAddress() (gas: 12116)
-QuarkWalletTest:testSetsMsgSender() (gas: 51196)
+QuarkWalletTest:testSetsMsgSender() (gas: 51165)
 QuarkWalletTest:testSetsMsgSenderDuringDirectExecute() (gas: 40265)
 RevertsTest:testRevertsInteger() (gas: 63758)
-RevertsTest:testRevertsInvalidOpcode() (gas: 8524964991080582761)
+RevertsTest:testRevertsInvalidOpcode() (gas: 8524964991080582766)
 RevertsTest:testRevertsOutOfGas() (gas: 295848)
 RevertsTest:testRevertsWhenDividingByZero() (gas: 63604)
-SupplyActionsTest:testInvalidInput() (gas: 80363)
-SupplyActionsTest:testRepayBorrow() (gas: 99871)
-SupplyActionsTest:testSupply() (gas: 142051)
-SupplyActionsTest:testSupplyFrom() (gas: 142407)
-SupplyActionsTest:testSupplyMultipleCollateral() (gas: 280880)
-SupplyActionsTest:testSupplyTo() (gas: 141737)
-TransferActionsTest:testRevertsForTransferReentrancyAttackWithReentrancyGuard() (gas: 163158)
-TransferActionsTest:testRevertsForTransferReentrancyAttackWithoutCallbackEnabled() (gas: 134432)
-TransferActionsTest:testRevertsForTransferReentrantAttackWithStolenSignature() (gas: 161030)
-TransferActionsTest:testTransferERC20TokenToEOA() (gas: 77675)
-TransferActionsTest:testTransferERC20TokenToQuarkWallet() (gas: 79104)
-TransferActionsTest:testTransferNativeTokenToEOA() (gas: 104987)
-TransferActionsTest:testTransferNativeTokenToQuarkWallet() (gas: 84776)
-TransferActionsTest:testTransferReentrancyAttackSuccessWithCallbackEnabled() (gas: 132159)
+SupplyActionsTest:testInvalidInput() (gas: 80325)
+SupplyActionsTest:testRepayBorrow() (gas: 99840)
+SupplyActionsTest:testSupply() (gas: 142020)
+SupplyActionsTest:testSupplyFrom() (gas: 142376)
+SupplyActionsTest:testSupplyMultipleCollateral() (gas: 280850)
+SupplyActionsTest:testSupplyTo() (gas: 141707)
+TransferActionsTest:testRevertsForTransferReentrancyAttackWithReentrancyGuard() (gas: 163120)
+TransferActionsTest:testRevertsForTransferReentrancyAttackWithoutCallbackEnabled() (gas: 134394)
+TransferActionsTest:testRevertsForTransferReentrantAttackWithStolenSignature() (gas: 160954)
+TransferActionsTest:testTransferERC20TokenToEOA() (gas: 77644)
+TransferActionsTest:testTransferERC20TokenToQuarkWallet() (gas: 79073)
+TransferActionsTest:testTransferNativeTokenToEOA() (gas: 104956)
+TransferActionsTest:testTransferNativeTokenToQuarkWallet() (gas: 84745)
+TransferActionsTest:testTransferReentrancyAttackSuccessWithCallbackEnabled() (gas: 132121)
 UniswapFlashLoanTest:testFlashLoanForCollateralSwapOnCompound() (gas: 422304)
 UniswapFlashLoanTest:testRevertsForInsufficientFundsToRepayFlashLoan() (gas: 189429)
 UniswapFlashLoanTest:testRevertsForInvalidCaller() (gas: 67226)
 UniswapFlashLoanTest:testTokensOrderInvariant() (gas: 91756)
 UniswapFlashSwapExactOutTest:testInvalidCallerFlashSwap() (gas: 67186)
 UniswapFlashSwapExactOutTest:testNotEnoughToPayFlashSwap() (gas: 289287)
 UniswapFlashSwapExactOutTest:testUniswapFlashSwapExactOutLeverageComet() (gas: 349543)
-UniswapSwapActionsTest:testBuyAssetOneStop() (gas: 272648)
-UniswapSwapActionsTest:testBuyAssetTwoStops() (gas: 385939)
-UniswapSwapActionsTest:testSellAssetOneStop() (gas: 270786)
-UniswapSwapActionsTest:testSellAssetTwoStops() (gas: 391358)
-WithdrawActionsTest:testBorrow() (gas: 160496)
-WithdrawActionsTest:testInvalidInput() (gas: 74030)
-WithdrawActionsTest:testWithdraw() (gas: 89063)
-WithdrawActionsTest:testWithdrawFrom() (gas: 88564)
-WithdrawActionsTest:testWithdrawMultipleAssets() (gas: 164689)
-WithdrawActionsTest:testWithdrawTo() (gas: 89044)
+UniswapSwapActionsTest:testBuyAssetOneStop() (gas: 272572)
+UniswapSwapActionsTest:testBuyAssetTwoStops() (gas: 385863)
+UniswapSwapActionsTest:testSellAssetOneStop() (gas: 270710)
+UniswapSwapActionsTest:testSellAssetTwoStops() (gas: 391282)
+WithdrawActionsTest:testBorrow() (gas: 160458)
+WithdrawActionsTest:testInvalidInput() (gas: 73992)
+WithdrawActionsTest:testWithdraw() (gas: 89032)
+WithdrawActionsTest:testWithdrawFrom() (gas: 88534)
+WithdrawActionsTest:testWithdrawMultipleAssets() (gas: 164659)
+WithdrawActionsTest:testWithdrawTo() (gas: 89014)
 isValidSignatureTest:testIsValidSignatureForEOAOwner() (gas: 13747)
 isValidSignatureTest:testReturnsMagicValueForValidSignature() (gas: 7168)
 isValidSignatureTest:testRevertsForEmptyContract() (gas: 9078)

src/CodeJar.sol

@@ -20,7 +20,10 @@ contract CodeJar {
         address codeAddress = getCodeAddress(initCode);
         bytes32 codeAddressHash = codeAddress.codehash;
 
-        if (codeAddressHash == 0) {
+        if (codeAddressHash == keccak256(code)) {
+            // Code is already deployed and matches expected code
+            return codeAddress;
+        } else {
             // The code has not been deployed here (or it was deployed and destructed).
             address codeCreateAddress;
             uint256 initCodeLen = initCode.length;
@@ -32,14 +35,6 @@ contract CodeJar {
             require(codeCreateAddress == codeAddress);
 
             return codeAddress;
-        } else if (codeAddressHash == keccak256(code)) {
-            // Code is already deployed and matches expected code
-            return codeAddress;
-        } else {
-            // Code is already deployed but does not match expected code.
-            // Note: this should never happen except if the initCode script
-            //       has an unknown bug.
-            revert CodeHashMismatch(codeAddress, keccak256(code), codeAddressHash);
         }
     }
 

test/CodeJar.t.sol

@@ -49,6 +49,28 @@ contract CodeJarTest is Test {
         assertApproxEqAbs(gasUsed, 42000, 3000);
     }
 
+    function testCodeJarDeployNotAffectedByChangedCodeHash() public {
+        vm.deal(address(0xbab), 10 ether);
+        bytes memory code = hex"11223344";
+        bytes memory initCode = abi.encodePacked(hex"63", uint32(code.length), hex"80600e6000396000f3", code);
+        address targetAddress = address(
+            uint160(
+                uint256(keccak256(abi.encodePacked(bytes1(0xff), address(codeJar), uint256(0), keccak256(initCode))))
+            )
+        );
+        vm.startPrank(address(0xbab));
+        // Attacker poison the target address so the codehash will be different
+        targetAddress.call{value: 1 ether}("");
+        vm.stopPrank();
+        assertNotEq(targetAddress.codehash, 0);
+        uint256 gasLeft = gasleft();
+        // CodeJar will detect the codehash diff, but it will still be able to deploy the code
+        address scriptAddress = codeJar.saveCode(code);
+        uint256 gasUsed = gasLeft - gasleft();
+        assertEq(scriptAddress.code, code);
+        assertApproxEqAbs(gasUsed, 40000, 3000);
+    }
+
     function testCodeJarSecondDeploy() public {
         address scriptAddress = codeJar.saveCode(hex"11223344");