Skip to content

Commit

Permalink
improve(SpokePool): Remove .transfer() (#308)
Browse files Browse the repository at this point in the history
* improve(SpokePool): Remove .transfer()

Uses safe, audited library function from openzeppelin instead of sending ETH via `.transfer()`, which is [not recommended](https://twitter.com/zksync/status/1644139364270878720) for zksync which has a very different gas fee model.

The library code can be found [here](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/AddressUpgradeable.sol#L41)

* wip

* wip

* fix

* WIP

* Re-add safety check for delegate call

* revert

* fix

* fix
  • Loading branch information
nicholaspai committed Jul 11, 2023
1 parent 5dfe663 commit 02cff2c
Show file tree
Hide file tree
Showing 15 changed files with 291 additions and 30 deletions.
7 changes: 3 additions & 4 deletions contracts/SpokePool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import "./external/interfaces/WETH9Interface.sol";
import "./interfaces/SpokePoolInterface.sol";
import "./upgradeable/MultiCallerUpgradeable.sol";
import "./upgradeable/EIP712CrossChainUpgradeable.sol";
import "./upgradeable/AddressLibUpgradeable.sol";

import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
import "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol";
import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
import "@openzeppelin/contracts/utils/math/SignedMath.sol";
Expand Down Expand Up @@ -43,7 +43,7 @@ abstract contract SpokePool is
EIP712CrossChainUpgradeable
{
using SafeERC20Upgradeable for IERC20Upgradeable;
using AddressUpgradeable for address;
using AddressLibUpgradeable for address;

// Address of the L1 contract that acts as the owner of this SpokePool. This should normally be set to the HubPool
// address. The crossDomainAdmin address is unused when the SpokePool is deployed to the same chain as the HubPool.
Expand Down Expand Up @@ -1100,8 +1100,7 @@ abstract contract SpokePool is
IERC20Upgradeable(address(wrappedNativeToken)).safeTransfer(to, amount);
} else {
wrappedNativeToken.withdraw(amount);
//slither-disable-next-line arbitrary-send-eth
to.transfer(amount);
AddressLibUpgradeable.sendValue(to, amount);
}
}

Expand Down
254 changes: 254 additions & 0 deletions contracts/upgradeable/AddressLibUpgradeable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,254 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.9.0) (utils/Address.sol)

pragma solidity ^0.8.1;

/**
* @title AddressUpgradeable
* @dev Collection of functions related to the address type
* @notice Logic is 100% copied from "@openzeppelin/contracts-upgradeable/contracts/utils/AddressUpgradeable.sol" but one
* comment is added to clarify why we allow delegatecall() in this contract, which is typically unsafe for use in
* upgradeable implementation contracts.
* @dev See https://docs.openzeppelin.com/upgrades-plugins/1.x/faq#delegatecall-selfdestruct for more details.
*/
library AddressLibUpgradeable {
/**
* @dev Returns true if `account` is a contract.
*
* [IMPORTANT]
* ====
* It is unsafe to assume that an address for which this function returns
* false is an externally-owned account (EOA) and not a contract.
*
* Among others, `isContract` will return false for the following
* types of addresses:
*
* - an externally-owned account
* - a contract in construction
* - an address where a contract will be created
* - an address where a contract lived, but was destroyed
*
* Furthermore, `isContract` will also return true if the target contract within
* the same transaction is already scheduled for destruction by `SELFDESTRUCT`,
* which only has an effect at the end of a transaction.
* ====
*
* [IMPORTANT]
* ====
* You shouldn't rely on `isContract` to protect against flash loan attacks!
*
* Preventing calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets
* like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract
* constructor.
* ====
*/
function isContract(address account) internal view returns (bool) {
// This method relies on extcodesize/address.code.length, which returns 0
// for contracts in construction, since the code is only stored at the end
// of the constructor execution.

return account.code.length > 0;
}

/**
* @dev Replacement for Solidity's `transfer`: sends `amount` wei to
* `recipient`, forwarding all available gas and reverting on errors.
*
* https://eips.ethereum.org/EIPS/eip-1884[EIP1884] increases the gas cost
* of certain opcodes, possibly making contracts go over the 2300 gas limit
* imposed by `transfer`, making them unable to receive funds via
* `transfer`. {sendValue} removes this limitation.
*
* https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more].
*
* IMPORTANT: because control is transferred to `recipient`, care must be
* taken to not create reentrancy vulnerabilities. Consider using
* {ReentrancyGuard} or the
* https://solidity.readthedocs.io/en/v0.8.0/security-considerations.html#use-the-checks-effects-interactions-pattern[checks-effects-interactions pattern].
*/
function sendValue(address payable recipient, uint256 amount) internal {
require(address(this).balance >= amount, "Address: insufficient balance");

(bool success, ) = recipient.call{ value: amount }("");
require(success, "Address: unable to send value, recipient may have reverted");
}

/**
* @dev Performs a Solidity function call using a low level `call`. A
* plain `call` is an unsafe replacement for a function call: use this
* function instead.
*
* If `target` reverts with a revert reason, it is bubbled up by this
* function (like regular Solidity function calls).
*
* Returns the raw returned data. To convert to the expected return value,
* use https://solidity.readthedocs.io/en/latest/units-and-global-variables.html?highlight=abi.decode#abi-encoding-and-decoding-functions[`abi.decode`].
*
* Requirements:
*
* - `target` must be a contract.
* - calling `target` with `data` must not revert.
*
* _Available since v3.1._
*/
function functionCall(address target, bytes memory data) internal returns (bytes memory) {
return functionCallWithValue(target, data, 0, "Address: low-level call failed");
}

/**
* @dev Same as {xref-Address-functionCall-address-bytes-}[`functionCall`], but with
* `errorMessage` as a fallback revert reason when `target` reverts.
*
* _Available since v3.1._
*/
function functionCall(
address target,
bytes memory data,
string memory errorMessage
) internal returns (bytes memory) {
return functionCallWithValue(target, data, 0, errorMessage);
}

/**
* @dev Same as {xref-Address-functionCall-address-bytes-}[`functionCall`],
* but also transferring `value` wei to `target`.
*
* Requirements:
*
* - the calling contract must have an ETH balance of at least `value`.
* - the called Solidity function must be `payable`.
*
* _Available since v3.1._
*/
function functionCallWithValue(
address target,
bytes memory data,
uint256 value
) internal returns (bytes memory) {
return functionCallWithValue(target, data, value, "Address: low-level call with value failed");
}

/**
* @dev Same as {xref-Address-functionCallWithValue-address-bytes-uint256-}[`functionCallWithValue`], but
* with `errorMessage` as a fallback revert reason when `target` reverts.
*
* _Available since v3.1._
*/
function functionCallWithValue(
address target,
bytes memory data,
uint256 value,
string memory errorMessage
) internal returns (bytes memory) {
require(address(this).balance >= value, "Address: insufficient balance for call");
(bool success, bytes memory returndata) = target.call{ value: value }(data);
return verifyCallResultFromTarget(target, success, returndata, errorMessage);
}

/**
* @dev Same as {xref-Address-functionCall-address-bytes-}[`functionCall`],
* but performing a static call.
*
* _Available since v3.3._
*/
function functionStaticCall(address target, bytes memory data) internal view returns (bytes memory) {
return functionStaticCall(target, data, "Address: low-level static call failed");
}

/**
* @dev Same as {xref-Address-functionCall-address-bytes-string-}[`functionCall`],
* but performing a static call.
*
* _Available since v3.3._
*/
function functionStaticCall(
address target,
bytes memory data,
string memory errorMessage
) internal view returns (bytes memory) {
(bool success, bytes memory returndata) = target.staticcall(data);
return verifyCallResultFromTarget(target, success, returndata, errorMessage);
}

/**
* @dev Same as {xref-Address-functionCall-address-bytes-}[`functionCall`],
* but performing a delegate call.
*
* _Available since v3.4._
*/
function functionDelegateCall(address target, bytes memory data) internal returns (bytes memory) {
return functionDelegateCall(target, data, "Address: low-level delegate call failed");
}

/**
* @dev Same as {xref-Address-functionCall-address-bytes-string-}[`functionCall`],
* but performing a delegate call.
*
* _Available since v3.4._
*/
function functionDelegateCall(
address target,
bytes memory data,
string memory errorMessage
) internal returns (bytes memory) {
/// @custom:oz-upgrades-unsafe-allow delegatecall
(bool success, bytes memory returndata) = target.delegatecall(data);
return verifyCallResultFromTarget(target, success, returndata, errorMessage);
}

/**
* @dev Tool to verify that a low level call to smart-contract was successful, and revert (either by bubbling
* the revert reason or using the provided one) in case of unsuccessful call or if target was not a contract.
*
* _Available since v4.8._
*/
function verifyCallResultFromTarget(
address target,
bool success,
bytes memory returndata,
string memory errorMessage
) internal view returns (bytes memory) {
if (success) {
if (returndata.length == 0) {
// only check isContract if the call was successful and the return data is empty
// otherwise we already know that it was a contract
require(isContract(target), "Address: call to non-contract");
}
return returndata;
} else {
_revert(returndata, errorMessage);
}
}

/**
* @dev Tool to verify that a low level call was successful, and revert if it wasn't, either by bubbling the
* revert reason or using the provided one.
*
* _Available since v4.3._
*/
function verifyCallResult(
bool success,
bytes memory returndata,
string memory errorMessage
) internal pure returns (bytes memory) {
if (success) {
return returndata;
} else {
_revert(returndata, errorMessage);
}
}

function _revert(bytes memory returndata, string memory errorMessage) private pure {
// Look for revert reason and bubble it up if present
if (returndata.length > 0) {
// The easiest way to bubble the revert reason is using memory via assembly
/// @solidity memory-safe-assembly
assembly {
let returndata_size := mload(returndata)
revert(add(32, returndata), returndata_size)
}
} else {
revert(errorMessage);
}
}
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"@ethersproject/abstract-signer": "5.7.0",
"@ethersproject/bignumber": "5.7.0",
"@openzeppelin/contracts": "4.9.2",
"@openzeppelin/contracts-upgradeable": "4.8.3",
"@openzeppelin/contracts-upgradeable": "4.9.2",
"@uma/common": "^2.29.0",
"@uma/contracts-node": "^0.3.18",
"@uma/core": "^2.41.0",
Expand Down
2 changes: 1 addition & 1 deletion test/SpokePool.Admin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe("SpokePool Admin Functions", async function () {
const spokePool = await hre.upgrades.deployProxy(
await getContractFactory("MockSpokePool", owner),
[1, owner.address, owner.address, owner.address],
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);
expect(await spokePool.numberOfDeposits()).to.equal(1);
});
Expand Down
1 change: 1 addition & 0 deletions test/SpokePool.Upgrades.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe("SpokePool Upgrade Functions", async function () {
it("Can upgrade", async function () {
const spokePoolV2 = await hre.upgrades.deployImplementation(await ethers.getContractFactory("MockSpokePoolV2"), {
kind: "uups",
unsafeAllow: ["delegatecall"],
});
const spokePoolV2Contract = (await ethers.getContractFactory("MockSpokePoolV2")).attach(spokePoolV2 as string);

Expand Down
8 changes: 4 additions & 4 deletions test/chain-specific-spokepools/Arbitrum_SpokePool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { hre } from "../../utils/utils.hre";
import { hubPoolFixture } from "../fixtures/HubPool.Fixture";
import { constructSingleRelayerRefundTree } from "../MerkleLib.utils";

let hubPool: Contract, arbitrumSpokePool: Contract, timer: Contract, dai: Contract, weth: Contract;
let hubPool: Contract, arbitrumSpokePool: Contract, dai: Contract, weth: Contract;
let l2Weth: string, l2Dai: string, crossDomainAliasAddress;

let owner: SignerWithAddress, relayer: SignerWithAddress, rando: SignerWithAddress, crossDomainAlias: SignerWithAddress;
Expand All @@ -24,7 +24,7 @@ let l2GatewayRouter: FakeContract;
describe("Arbitrum Spoke Pool", function () {
beforeEach(async function () {
[owner, relayer, rando] = await ethers.getSigners();
({ weth, l2Weth, dai, l2Dai, hubPool, timer } = await hubPoolFixture());
({ weth, l2Weth, dai, l2Dai, hubPool } = await hubPoolFixture());

// Create an alias for the Owner. Impersonate the account. Crate a signer for it and send it ETH.
crossDomainAliasAddress = avmL1ToL2Alias(owner.address);
Expand All @@ -37,7 +37,7 @@ describe("Arbitrum Spoke Pool", function () {
arbitrumSpokePool = await hre.upgrades.deployProxy(
await getContractFactory("Arbitrum_SpokePool", owner),
[0, l2GatewayRouter.address, owner.address, hubPool.address, l2Weth],
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);

await seedContract(arbitrumSpokePool, relayer, [dai], weth, amountHeldByPool);
Expand All @@ -48,7 +48,7 @@ describe("Arbitrum Spoke Pool", function () {
// TODO: Could also use upgrades.prepareUpgrade but I'm unclear of differences
const implementation = await hre.upgrades.deployImplementation(
await getContractFactory("Arbitrum_SpokePool", owner),
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);

// upgradeTo fails unless called by cross domain admin
Expand Down
8 changes: 4 additions & 4 deletions test/chain-specific-spokepools/Ethereum_SpokePool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@ import { hre } from "../../utils/utils.hre";
import { hubPoolFixture } from "../fixtures/HubPool.Fixture";
import { constructSingleRelayerRefundTree } from "../MerkleLib.utils";

let hubPool: Contract, spokePool: Contract, timer: Contract, dai: Contract, weth: Contract;
let hubPool: Contract, spokePool: Contract, dai: Contract, weth: Contract;

let owner: SignerWithAddress, relayer: SignerWithAddress, rando: SignerWithAddress;

describe("Ethereum Spoke Pool", function () {
beforeEach(async function () {
[owner, relayer, rando] = await ethers.getSigners();
({ weth, dai, hubPool, timer } = await hubPoolFixture());
({ weth, dai, hubPool } = await hubPoolFixture());

spokePool = await hre.upgrades.deployProxy(
await getContractFactory("Ethereum_SpokePool", owner),
[0, hubPool.address, weth.address],
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);

// Seed spoke pool with tokens that it should transfer to the hub pool
Expand All @@ -28,7 +28,7 @@ describe("Ethereum Spoke Pool", function () {
// TODO: Could also use upgrades.prepareUpgrade but I'm unclear of differences
const implementation = await hre.upgrades.deployImplementation(
await getContractFactory("Ethereum_SpokePool", owner),
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);

// upgradeTo fails unless called by cross domain admin
Expand Down
4 changes: 2 additions & 2 deletions test/chain-specific-spokepools/Optimism_SpokePool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe("Optimism Spoke Pool", function () {
optimismSpokePool = await hre.upgrades.deployProxy(
await getContractFactory("MockOptimism_SpokePool", owner),
[weth.address, l2Eth, 0, owner.address, hubPool.address],
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);

await seedContract(optimismSpokePool, relayer, [dai], weth, amountHeldByPool);
Expand All @@ -50,7 +50,7 @@ describe("Optimism Spoke Pool", function () {
// TODO: Could also use upgrades.prepareUpgrade but I'm unclear of differences
const implementation = await hre.upgrades.deployImplementation(
await getContractFactory("Optimism_SpokePool", owner),
{ kind: "uups" }
{ kind: "uups", unsafeAllow: ["delegatecall"] }
);

// upgradeTo fails unless called by cross domain admin via messenger contract
Expand Down
Loading

0 comments on commit 02cff2c

Please sign in to comment.