Skip to content

N
nebula
Commit 329d21b6 authored by Mark Tyneway's avatar Mark Tyneway
Browse files
Options

contracts-bedrock: use safecall 

Replace `ExcessivelySafeCall` with a local `SafeCall`
that does not copy any calldata back. We do not care about
returndata being returned so this prevents returndata bombs.
parent 11123565
Hide whitespace changes
Inline Side-by-side
Showing with 120 additions and 40 deletions
.changeset/shaggy-ladybugs-arrive.md 0 → 100644
View file @ 329d21b6
---
'@eth-optimism/contracts-bedrock': patch
---
Use safecall that doesn't copy calldata
packages/contracts-bedrock/.gas-snapshot
View file @ 329d21b6
......@@ -4,7 +4,7 @@ GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 3544
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 118592)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 354436)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 118567)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 45432)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 45250)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 68671)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 74964)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 35777)
......@@ -37,9 +37,9 @@ L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 24515)
L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 24562)
L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 24716)
L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 48053)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 201827)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 195126)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 77865)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 201405)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 194907)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 77654)
L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 68005)
L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 60526)
L1CrossDomainMessenger_Test:test_L1MessengerReplayMessageWithValue() (gas: 38193)
......@@ -47,14 +47,14 @@ L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 301590)
L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1492584)
L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 40948)
L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 24305)
L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 86378)
L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 86170)
L1StandardBridge_Test:test_depositERC20() (gas: 579126)
L1StandardBridge_Test:test_depositERC20To() (gas: 581333)
L1StandardBridge_Test:test_depositETH() (gas: 373955)
L1StandardBridge_Test:test_depositETHTo() (gas: 331091)
L1StandardBridge_Test:test_finalizeBridgeERC20FailSendBack() (gas: 680116)
L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 489836)
L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 64273)
L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 64091)
L1StandardBridge_Test:test_initialize() (gas: 26334)
L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 22376)
L1StandardBridge_Test:test_onlyEOADepositETH() (gas: 40859)
......@@ -62,17 +62,17 @@ L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 36268)
L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 35573)
L1StandardBridge_Test:test_receive() (gas: 520573)
L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10860)
L2CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 171954)
L2CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 171735)
L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8411)
L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31797)
L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 172968)
L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57396)
L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 172546)
L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57185)
L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 36217)
L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41682)
L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 122833)
L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 138556)
L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10598)
L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54889)
L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54681)
L2OutputOracleTest:testCannot_ProposeWithUnmatchedBlockhash() (gas: 26829)
L2OutputOracleTest:testCannot_deleteL2Output_ifNotOwner() (gas: 25123)
L2OutputOracleTest:testCannot_deleteL2Output_withWrongRoot() (gas: 91445)
......@@ -138,12 +138,12 @@ OptimismPortalUpgradeable_Test:test_upgrading() (gas: 180632)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnInsufficientGas() (gas: 160788)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnInvalidOutputRootProof() (gas: 81241)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnRecentWithdrawal() (gas: 53001)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnReentrancy() (gas: 205566)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnReplay() (gas: 278941)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnReentrancy() (gas: 205354)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnReplay() (gas: 278729)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOnSelfCall() (gas: 50539)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_revertsOninvalidWithdrawalProof() (gas: 148562)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_succeeds() (gas: 187013)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails() (gas: 289662)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_succeeds() (gas: 186801)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails() (gas: 289450)
OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17341)
OptimismPortal_Test:test_OptimismPortalContractCreationReverts() (gas: 14199)
OptimismPortal_Test:test_OptimismPortalReceiveEth() (gas: 127534)
......
packages/contracts-bedrock/contracts/L1/OptimismPortal.sol
View file @ 329d21b6
......@@ -2,7 +2,7 @@
pragma solidity 0.8.15;
import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import { ExcessivelySafeCall } from "excessively-safe-call/src/ExcessivelySafeCall.sol";
import { SafeCall } from "../libraries/SafeCall.sol";
import { L2OutputOracle } from "./L2OutputOracle.sol";
import { Types } from "../libraries/Types.sol";
import { Hashing } from "../libraries/Hashing.sol";
......@@ -195,16 +195,10 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
// Set the l2Sender so contracts know who triggered this withdrawal on L2.
l2Sender = _tx.sender;
// Trigger the call to the target contract. We use excessivelySafeCall because we don't
// Trigger the call to the target contract. We use SafeCall because we don't
// care about the returndata and we don't want target contracts to be able to force this
// call to run out of gas.
(bool success, ) = ExcessivelySafeCall.excessivelySafeCall(
_tx.target,
_tx.gasLimit,
_tx.value,
0,
_tx.data
);
// call to run out of gas via a returndata bomb.
bool success = SafeCall.call(_tx.target, _tx.gasLimit, _tx.value, _tx.data);
// Reset the l2Sender back to the default value.
l2Sender = DEFAULT_L2_SENDER;
......
packages/contracts-bedrock/contracts/libraries/SafeCall.sol 0 → 100644
View file @ 329d21b6
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
/**
* @title SafeCall
* @notice Perform low level safe calls
*/
library SafeCall {
/**
* @notice Perform a low level call without copying any returndata
*
* @param _target Address to call
* @param _gas Amount of gas to pass to the call
* @param _value Amount of value to pass to the call
* @param _calldata Calldata to pass to the call
*/
function call(
address _target,
uint256 _gas,
uint256 _value,
bytes memory _calldata
) internal returns (bool) {
bool _success;
assembly {
_success := call(
_gas, // gas
_target, // recipient
_value, // ether value
add(_calldata, 0x20), // inloc
mload(_calldata), // inlen
0, // outloc
0 // outlen
)
}
return _success;
}
}
packages/contracts-bedrock/contracts/test/SafeCall.t.sol 0 → 100644
View file @ 329d21b6
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
import { CommonTest } from "./CommonTest.t.sol";
import { SafeCall } from "../libraries/SafeCall.sol";
contract SafeCall_Test is CommonTest {
function test_safeCall(
address from,
address to,
uint256 gas,
uint64 value,
bytes memory data
) external {
vm.assume(from.balance == 0);
vm.assume(to.balance == 0);
// no precompiles
vm.assume(uint160(to) > 10);
// don't call the vm
vm.assume(to != address(vm));
vm.assume(from != address(vm));
// don't call the console
vm.assume(
to != address(0x000000000000000000636F6e736F6c652e6c6f67)
);
// don't call the create2 deployer
vm.assume(
to != address(0x4e59b44847b379578588920cA78FbF26c0B4956C)
);
// don't send funds to self
vm.assume(from != to);
assertEq(from.balance, 0, "from balance is 0");
vm.deal(from, value);
assertEq(from.balance, value, "from balance not dealt");
vm.expectCall(
to,
value,
data
);
vm.prank(from);
bool success = SafeCall.call(
to,
gas,
value,
data
);
assertEq(success, true, "call not successful");
assertEq(to.balance, value, "to balance received");
assertEq(from.balance, 0, "from balance not drained");
}
}
packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
View file @ 329d21b6
......@@ -10,7 +10,7 @@ import {
import {
ReentrancyGuardUpgradeable
} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
import { ExcessivelySafeCall } from "excessively-safe-call/src/ExcessivelySafeCall.sol";
import { SafeCall } from "../libraries/SafeCall.sol";
import { Hashing } from "../libraries/Hashing.sol";
import { Encoding } from "../libraries/Encoding.sol";
......@@ -269,13 +269,7 @@ abstract contract CrossDomainMessenger is
);
xDomainMsgSender = _sender;
(bool success, ) = ExcessivelySafeCall.excessivelySafeCall(
_target,
gasleft() - RELAY_GAS_BUFFER,
_value,
0,
_message
);
bool success = SafeCall.call(_target, gasleft() - RELAY_GAS_BUFFER, _value, _message);
xDomainMsgSender = DEFAULT_XDOMAIN_SENDER;
if (success == true) {
......
packages/contracts-bedrock/contracts/universal/StandardBridge.sol
View file @ 329d21b6
......@@ -5,6 +5,7 @@ import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import { Address } from "@openzeppelin/contracts/utils/Address.sol";
import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import { SafeCall } from "../libraries/SafeCall.sol";
import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import { IRemoteToken, IL1Token } from "./SupportedInterfaces.sol";
import { CrossDomainMessenger } from "./CrossDomainMessenger.sol";
......@@ -284,7 +285,7 @@ abstract contract StandardBridge is Initializable {
require(_to != address(this), "StandardBridge: cannot send to self");
emit ETHBridgeFinalized(_from, _to, _amount, _extraData);
(bool success, ) = _to.call{ value: _amount }(new bytes(0));
bool success = SafeCall.call(_to, gasleft(), _amount, hex"");
require(success, "StandardBridge: ETH transfer failed");
}
......
packages/contracts-bedrock/foundry.toml
View file @ 329d21b6
......@@ -8,7 +8,6 @@ remappings = [
'@openzeppelin/contracts-upgradeable/=node_modules/@openzeppelin/contracts-upgradeable/',
'@openzeppelin/contracts/=node_modules/@openzeppelin/contracts/',
'@eth-optimism/contracts-periphery/=node_modules/@eth-optimism/contracts-periphery/contracts',
'excessively-safe-call/=node_modules/excessively-safe-call/',
'@rari-capital/solmate/=node_modules/@rari-capital/solmate',
'forge-std/=node_modules/forge-std/src',
'ds-test/=node_modules/ds-test/src'
......
packages/contracts-bedrock/package.json
View file @ 329d21b6
......@@ -40,9 +40,7 @@
"@eth-optimism/core-utils": "^0.9.3",
"@openzeppelin/contracts": "4.7.3",
"@openzeppelin/contracts-upgradeable": "4.7.3",
"@rari-capital/solmate": "https://github.com/rari-capital/solmate.git#8f9b23f8838670afda0fd8983f2c41e8037ae6bc",
"ethers": "^5.6.8",
"excessively-safe-call": "https://github.com/nomad-xyz/ExcessivelySafeCall.git#81cd99ce3e69117d665d7601c330ea03b97acce0",
"hardhat": "^2.9.6"
},
"devDependencies": {
......@@ -55,6 +53,7 @@
"@foundry-rs/easy-foundryup": "^0.1.3",
"@nomiclabs/hardhat-ethers": "^2.0.0",
"@nomiclabs/hardhat-waffle": "^2.0.0",
"@rari-capital/solmate": "https://github.com/rari-capital/solmate.git#8f9b23f8838670afda0fd8983f2c41e8037ae6bc",
"@typechain/ethers-v5": "^10.1.0",
"@typescript-eslint/eslint-plugin": "^5.26.0",
"@typescript-eslint/parser": "^4.29.1",
......
yarn.lock
View file @ 329d21b6
......@@ -8608,10 +8608,6 @@ evp_bytestokey@^1.0.0, evp_bytestokey@^1.0.3:
md5.js "^1.3.4"
safe-buffer "^5.1.1"
"excessively-safe-call@https://github.com/nomad-xyz/ExcessivelySafeCall.git#81cd99ce3e69117d665d7601c330ea03b97acce0":
version "0.0.1-rc.1"
resolved "https://github.com/nomad-xyz/ExcessivelySafeCall.git#81cd99ce3e69117d665d7601c330ea03b97acce0"
execa@^0.7.0:
version "0.7.0"
resolved "https://registry.yarnpkg.com/execa/-/execa-0.7.0.tgz#944becd34cc41ee32a63a9faf27ad5a65fc59777"
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment