Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use transient storage for reentrancy guards #96

Draft
wants to merge 18 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions contracts/tokenbridge/ethereum/L1AtomicTokenBridgeCreator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@ import {
ProxyAdmin
} from "../arbitrum/L2AtomicTokenBridgeFactory.sol";
import {CreationCodeHelper} from "../libraries/CreationCodeHelper.sol";
import {
IUpgradeExecutor,
UpgradeExecutor
} from "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol";
import {IUpgradeExecutor} from "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol";
import {AddressAliasHelper} from "../libraries/AddressAliasHelper.sol";
import {IInbox, IBridge, IOwnable} from "@arbitrum/nitro-contracts/src/bridge/IInbox.sol";
import {ArbMulticall2} from "../../rpc-utils/MulticallV2.sol";
Expand Down Expand Up @@ -205,7 +202,7 @@ contract L1AtomicTokenBridgeCreator is Initializable, OwnableUpgradeable {
address upgradeExecutor = IInbox(inbox).bridge().rollup().owner();
if (
!IAccessControlUpgradeable(upgradeExecutor).hasRole(
UpgradeExecutor(upgradeExecutor).EXECUTOR_ROLE(), rollupOwner
IUpgradeExecutor(upgradeExecutor).EXECUTOR_ROLE(), rollupOwner
)
) {
revert L1AtomicTokenBridgeCreator_RollupOwnershipMisconfig();
Expand Down
19 changes: 9 additions & 10 deletions contracts/tokenbridge/ethereum/gateway/L1CustomGateway.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* limitations under the License.
*/

pragma solidity ^0.8.0;
pragma solidity ^0.8.28;

import { ArbitrumEnabledToken } from "../ICustomToken.sol";
import "./L1ArbitrumExtendedGateway.sol";
Expand All @@ -41,18 +41,19 @@ contract L1CustomGateway is L1ArbitrumExtendedGateway, ICustomGateway {
address public whitelist;

// start of inline reentrancy guard
// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.2/contracts/utils/ReentrancyGuard.sol
uint256 private constant _NOT_ENTERED = 1;
uint256 private constant _ENTERED = 2;
uint256 private _status;
// @dev deprecated in place of transient storage
uint256 private __status;

// @dev use transient storage for reentrancy check
uint256 private transient reentrancyStatus;

modifier nonReentrant() {
// On the first call to nonReentrant, _notEntered will be true
require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
// Any calls to nonReentrant after this point will fail
_status = _ENTERED;
require(reentrancyStatus != _ENTERED, "ReentrancyGuard: reentrant call");
reentrancyStatus = _ENTERED;
_;
_status = _NOT_ENTERED;
reentrancyStatus = _NOT_ENTERED;
}

modifier onlyOwner() {
Expand Down Expand Up @@ -102,8 +103,6 @@ contract L1CustomGateway is L1ArbitrumExtendedGateway, ICustomGateway {
owner = _owner;
// disable whitelist by default
whitelist = address(0);
// reentrancy guard
_status = _NOT_ENTERED;
}

/**
Expand Down
21 changes: 9 additions & 12 deletions contracts/tokenbridge/ethereum/gateway/L1ERC20Gateway.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* limitations under the License.
*/

pragma solidity ^0.8.0;
pragma solidity ^0.8.28;

import "./L1ArbitrumExtendedGateway.sol";
import "@openzeppelin/contracts/utils/Create2.sol";
Expand All @@ -38,22 +38,21 @@ contract L1ERC20Gateway is L1ArbitrumExtendedGateway {
address public whitelist;

// start of inline reentrancy guard
// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.2/contracts/utils/ReentrancyGuard.sol
uint256 private constant _NOT_ENTERED = 1;
uint256 private constant _ENTERED = 2;
uint256 private _status;
// @dev deprecated in place of transient storage
uint256 private __status;

// @dev use transient storage for reentrancy check
uint256 private transient reentrancyStatus;

modifier nonReentrant() {
// On the first call to nonReentrant, _notEntered will be true
require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
// Any calls to nonReentrant after this point will fail
_status = _ENTERED;
require(reentrancyStatus != _ENTERED, "ReentrancyGuard: reentrant call");
reentrancyStatus = _ENTERED;
_;
_status = _NOT_ENTERED;
reentrancyStatus = _NOT_ENTERED;
}

// end of inline reentrancy guard

function outboundTransferCustomRefund(
address _l1Token,
address _refundTo,
Expand Down Expand Up @@ -100,8 +99,6 @@ contract L1ERC20Gateway is L1ArbitrumExtendedGateway {
l2BeaconProxyFactory = _l2BeaconProxyFactory;
// disable whitelist by default
whitelist = address(0);
// reentrancy guard
_status = _NOT_ENTERED;
}

/**
Expand Down
12 changes: 6 additions & 6 deletions contracts/tokenbridge/libraries/CreationCodeHelper.sol
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.0;
pragma solidity 0.8.28;

library CreationCodeHelper {
/**
* @notice Generate a creation code that results with a contract with `code` as deployed code.
* @notice Generate a creation code that results with a contract with `runtimeCode` as deployed code.
* Generated creation code shall match the one generated by Solidity compiler with an empty constructor.
* @dev Prepended constructor bytecode consists of:
* - 608060405234801561001057600080fd5b50 - store free memory pointer, then check no callvalue is provided
* - 6080604052348015600e575f5ffd5b50 - store free memory pointer, then check no callvalue is provided
* - 61xxxx - push 2 bytes of `code` length
* - 806100206000396000f3fe - copy deployed code to memory and return the location of it
* - 8061001c5f395ff3fe - copy deployed code to memory and return the location of it
* @param runtimeCode Deployed bytecode to which constructor bytecode will be prepended
* @return Creation code of a new contract
*/
function getCreationCodeFor(bytes memory runtimeCode) internal pure returns (bytes memory) {
return abi.encodePacked(
hex"608060405234801561001057600080fd5b50",
hex"6080604052348015600e575f5ffd5b50",
hex"61",
uint16(runtimeCode.length),
hex"806100206000396000f3fe",
hex"8061001c5f395ff3fe",
runtimeCode
);
}
Expand Down
3 changes: 2 additions & 1 deletion foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ cache_path = 'forge-cache'
optimizer = true
optimizer_runs = 100
via_ir = false
fs_permissions = [{ access = "read", path = "node_modules/@offchainlabs/stablecoin-evm"}]
evm_version = 'cancun'
fs_permissions = [{ access = "read", path = "node_modules/@offchainlabs"}]

[fmt]
number_underscore = 'thousands'
Expand Down
3 changes: 2 additions & 1 deletion hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ const config = {
},
},
{
version: '0.8.16',
version: '0.8.28',
settings: {
evmVersion: 'cancun',
optimizer: {
enabled: true,
runs: 100,
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"dependencies": {
"@arbitrum/nitro-contracts": "1.1.1",
"@offchainlabs/stablecoin-evm": "1.0.0-orbit-alpha.2",
"@offchainlabs/upgrade-executor": "1.1.0-beta.0",
"@offchainlabs/upgrade-executor": "^1.1.1",
"@openzeppelin/contracts": "4.8.3",
"@openzeppelin/contracts-upgradeable": "4.8.3"
},
Expand Down Expand Up @@ -74,7 +74,7 @@
"ethereum-waffle": "^3.2.0",
"ethers": "^5.4.5",
"fs-extra": "^11.2.0",
"hardhat": "2.17.3",
"hardhat": "^2.22.13",
"hardhat-contract-sizer": "^2.10.0",
"hardhat-deploy": "^0.9.1",
"hardhat-gas-reporter": "^1.0.4",
Expand Down
2 changes: 1 addition & 1 deletion slither.db.json

Large diffs are not rendered by default.

35 changes: 25 additions & 10 deletions test-e2e/creationCodeTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import fs from 'fs'

const LOCALHOST_L2_RPC = 'http://127.0.0.1:8547'

const AE_WETH_EXPECTED_CONSTRUCTOR_SIZE = 348
const AE_WETH_EXPECTED_CONSTRUCTOR_SIZE = 290
const UPGRADE_EXECUTOR_EXPECTED_CONSTRUCTOR_SIZE = 242

let provider: JsonRpcProvider
Expand Down Expand Up @@ -114,7 +114,14 @@ describe('creationCodeTest', () => {
})

it('aeWETH constructor has expected size', async function () {
const constructorBytecode = await _getConstructorBytecode('aeWETH')
const artifact = await hre.artifacts.readArtifact('aeWETH')
const creationCode = artifact.bytecode.substring(2)
const runtimeCode = artifact.deployedBytecode.substring(2)

const constructorBytecode = await _getConstructorBytecode(
creationCode,
runtimeCode
)
const constructorBytecodeLength = _lengthInBytes(constructorBytecode)

expect(constructorBytecodeLength).to.be.eq(
Expand All @@ -123,7 +130,18 @@ describe('creationCodeTest', () => {
})

it('UpgradeExecutor constructor has expected size', async function () {
const constructorBytecode = await _getConstructorBytecode('@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor')
const bytecodePath = path.join(
__dirname,
'../node_modules/@offchainlabs/upgrade-executor/build/contracts/src/UpgradeExecutor.sol/UpgradeExecutor.json'
)
const artifact = JSON.parse(fs.readFileSync(bytecodePath, 'utf-8'))
const creationCode = artifact.bytecode.substring(2)
const runtimeCode = artifact.deployedBytecode.substring(2)

const constructorBytecode = await _getConstructorBytecode(
creationCode,
runtimeCode
)
const constructorBytecodeLength = _lengthInBytes(constructorBytecode)

expect(constructorBytecodeLength).to.be.eq(
Expand Down Expand Up @@ -171,13 +189,10 @@ async function _getTokenBridgeCreator(
* @param contractName
* @returns
*/
async function _getConstructorBytecode(contractName: string): Promise<string> {
const artifact = await hre.artifacts.readArtifact(contractName)

// remove '0x'
const creationCode = artifact.bytecode.substring(2)
const runtimeCode = artifact.deployedBytecode.substring(2)

async function _getConstructorBytecode(
creationCode: string,
runtimeCode: string
): Promise<string> {
if (!creationCode.includes(runtimeCode)) {
throw new Error(
`Error while extracting constructor bytecode for contract ${contractName}.`
Expand Down
60 changes: 42 additions & 18 deletions test-foundry/AtomicTokenBridgeFactory.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@ import "forge-std/Test.sol";
import "../contracts/tokenbridge/ethereum/L1AtomicTokenBridgeCreator.sol";
import "../contracts/tokenbridge/arbitrum/L2AtomicTokenBridgeFactory.sol";
import "../contracts/tokenbridge/libraries/AddressAliasHelper.sol";

import {L1TokenBridgeRetryableSender} from
"../contracts/tokenbridge/ethereum/L1TokenBridgeRetryableSender.sol";
import {TestWETH9} from "../contracts/tokenbridge/test/TestWETH9.sol";
import {Multicall2} from "../contracts/rpc-utils/MulticallV2.sol";

import {TransparentUpgradeableProxy} from
"@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

Expand Down Expand Up @@ -55,15 +53,15 @@ contract MockInbox is Test {
return address(this);
}

function hasRole(bytes32, address) external view returns (bool) {
function hasRole(bytes32, address) external pure returns (bool) {
return true;
}

function getProxyAdmin() external view returns (address) {
return address(this);
}

function calculateRetryableSubmissionFee(uint256, uint256) external view returns (uint256) {
function calculateRetryableSubmissionFee(uint256, uint256) external pure returns (uint256) {
return 0;
}

Expand Down Expand Up @@ -105,6 +103,7 @@ contract MockInbox is Test {
}
}
vm.stopPrank();
return 0;
}
}

Expand Down Expand Up @@ -138,8 +137,9 @@ contract AtomicTokenBridgeCreatorTest is Test {
L1OrbitGatewayRouter(address(new L1OrbitGatewayRouter())),
L1OrbitERC20Gateway(address(new L1OrbitERC20Gateway())),
L1OrbitCustomGateway(address(new L1OrbitCustomGateway())),
IUpgradeExecutor(address(new UpgradeExecutor()))
_deployUpgradeExecutor()
);

l2TokenBridgeFactoryTemplate = address(new L2AtomicTokenBridgeFactory());
l2RouterTemplate = address(new L2GatewayRouter());
l2StandardGatewayTemplate = address(new L2ERC20Gateway());
Expand Down Expand Up @@ -219,13 +219,13 @@ contract AtomicTokenBridgeCreatorTest is Test {
{
(address l1r, address l1sgw, address l1cgw, address l1wgw, address l1w) =
factory.inboxToL1Deployment(address(inbox));
assertEq(l1r, 0xcB37BCa7042A10FfA75Ff95Ad8B361A13bbAA63A, "l1r");
assertEq(l1r, 0xb458B5E13BEf3ca7C4a87bF7368D3Fe9E7a631DE, "l1r");
assertTrue(l1r.code.length > 0, "l1r code");
assertEq(l1sgw, 0x013b54d88f76fb9D05b8382747beb1B4Df313507, "l1sgw");
assertEq(l1sgw, 0x75b043aA7C73F9c3eC8D3A1A635a39b30E93cf2f, "l1sgw");
assertTrue(l1sgw.code.length > 0, "l1sgw code");
assertEq(l1cgw, 0xf8663294698E0623de82B9791906454A2036575F, "l1cgw");
assertEq(l1cgw, 0x5Edd3097a2a1fE878E61efB2F3FFA41BDD58803E, "l1cgw");
assertTrue(l1cgw.code.length > 0, "l1cgw code");
assertEq(l1wgw, 0x79eF26bE05C5643D5AdC81B8c7e49b0898A74428, "l1wgw");
assertEq(l1wgw, 0x9062A6901a9E0285dcd67d14046412be48db143f, "l1wgw");
assertTrue(l1wgw.code.length > 0, "l1wgw code");
assertEq(l1w, 0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758, "l1w");
assertTrue(l1w.code.length > 0, "l1w code");
Expand All @@ -243,24 +243,48 @@ contract AtomicTokenBridgeCreatorTest is Test {
address l2mc
) = factory.inboxToL2Deployment(address(inbox));

assertEq(l2r, 0xdB4050B663976d45E810B7C0E3B8B25564bD620d, "l2r");
assertEq(l2r, 0xcf35298BFB982476a85201Eb144F7099761744Ee, "l2r");
assertTrue(l2r.code.length > 0, "l2r code");
assertEq(l2sgw, 0x25F753b06E1e092292e6773E119D00BEe5A1b8D4, "l2sgw");
assertEq(l2sgw, 0x48d1FE88d96c0Cb40B0ddD660a0dd7edE13517C9, "l2sgw");
assertTrue(l2sgw.code.length > 0, "l2sgw code");
assertEq(l2cgw, 0x4Ca25428D90D0813EC134b5160eb6301909B4A9B, "l2cgw");
assertEq(l2cgw, 0x7C8118742fB20e0A786C5C21cbf7c59c412130bD, "l2cgw");
assertTrue(l2cgw.code.length > 0, "l2cgw code");
assertEq(l2wgw, 0x29B1Fa62Af163E550Cb4173BE58787fa2d6456fF, "l2wgw");
assertEq(l2wgw, 0x05d35724540FD8E4149933065f53431dfDcF4e80, "l2wgw");
assertTrue(l2wgw.code.length > 0, "l2wgw code");
assertEq(l2w, 0x7C9c18AE0EeA13600496D1222E8Ec22738b29C61, "l2w");
assertEq(l2w, 0x9bd4C8C3D644D8036a09726A5044d09cB33a1E3e, "l2w");
assertTrue(l2w.code.length > 0, "l2w code");
assertEq(l2pa, 0xf789F48Bc2c9ee6E98E564E6383B394ba6F9378c, "l2pa");
assertEq(l2pa, 0x5C8fEE06019d8E1E1EF424C867f8A40885214aFB, "l2pa");
assertTrue(l2pa.code.length > 0, "l2pa code");
assertEq(l2bpf, 0x9446B15B1128aD326Ccf310a68F2FFB652D31934, "l2bpf");
assertEq(l2bpf, 0xa744250a6CA35F6DB35B001AC5aa1E76A7D312CE, "l2bpf");
assertTrue(l2bpf.code.length > 0, "l2bpf code");
assertEq(l2ue, 0xC85c71251E9354Cd6a8992BC02d968B04F4b55e6, "l2ue");
assertEq(l2ue, 0x297eA477216C8E118278cB1D91D2A1dE761460f6, "l2ue");
assertTrue(l2ue.code.length > 0, "l2ue code");
assertEq(l2mc, 0x6466F88A4E3B536892e706258c1079D0a880d7Cb, "l2mc");
assertEq(l2mc, 0xfb61c207b20a581FA4559f90C1C50eb356c3BE10, "l2mc");
assertTrue(l2mc.code.length > 0, "l2mc code");
}
}

function _deployUpgradeExecutor() internal returns (IUpgradeExecutor executor) {
bytes memory bytecode = _getBytecode(
"/node_modules/@offchainlabs/upgrade-executor/build/contracts/src/UpgradeExecutor.sol/UpgradeExecutor.json"
);

address addr;
assembly {
addr := create(0, add(bytecode, 0x20), mload(bytecode))
}
require(addr != address(0), "bytecode deployment failed");

executor = IUpgradeExecutor(addr);
}

function _getBytecode(bytes memory path) internal returns (bytes memory) {
string memory readerBytecodeFilePath = string(abi.encodePacked(vm.projectRoot(), path));
string memory json = vm.readFile(readerBytecodeFilePath);
try vm.parseJsonBytes(json, ".bytecode.object") returns (bytes memory bytecode) {
return bytecode;
} catch {
return vm.parseJsonBytes(json, ".bytecode");
}
}
}
2 changes: 1 addition & 1 deletion test-foundry/L1ArbitrumGateway.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ abstract contract L1ArbitrumGatewayTest is Test {
contract L1ArbitrumGatewayMock is L1ArbitrumGateway {
function calculateL2TokenAddress(address x)
public
view
pure
override(ITokenGateway, TokenGateway)
returns (address)
{
Expand Down
Loading
Loading