Skip to content

Commit

Permalink
Audit/dapphub b02 gas relaying check (#85)
Browse files Browse the repository at this point in the history
* add SafeMath, add dummy check for gas sufficiency

* gas_overhead const

* custom safemath

* add gasLimit param for testing, safemath credit
  • Loading branch information
axchu authored Dec 2, 2020
1 parent 15cc605 commit 0c3c650
Show file tree
Hide file tree
Showing 3 changed files with 218 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ import { iOVM_ECDSAContractAccount } from "../../iOVM/accounts/iOVM_ECDSAContrac
import { Lib_OVMCodec } from "../../libraries/codec/Lib_OVMCodec.sol";
import { Lib_ECDSAUtils } from "../../libraries/utils/Lib_ECDSAUtils.sol";
import { Lib_SafeExecutionManagerWrapper } from "../../libraries/wrappers/Lib_SafeExecutionManagerWrapper.sol";
import { Lib_SafeMathWrapper } from "../../libraries/wrappers/Lib_SafeMathWrapper.sol";

/**
* @title OVM_ECDSAContractAccount
*/
contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {

address constant ETH_ERC20_ADDRESS = 0x4200000000000000000000000000000000000006;
uint256 constant EXECUTION_VALIDATION_GAS_OVERHEAD = 25000; // TODO: should be the amount sufficient to cover the gas costs of all of the transactions up to and including the CALL/CREATE which forms the entrypoint of the transaction.

/********************
* Public Functions *
Expand Down Expand Up @@ -76,6 +78,12 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
"Transaction nonce does not match the expected nonce."
);

// Need to make sure that the gas is sufficient to execute the transaction.
Lib_SafeExecutionManagerWrapper.safeREQUIRE(
gasleft() >= Lib_SafeMathWrapper.add(decodedTx.gasLimit, EXECUTION_VALIDATION_GAS_OVERHEAD),
"Gas is not sufficient to execute the transaction."
);

// Transfer fee to relayer.
address relayer = Lib_SafeExecutionManagerWrapper.safeCALLER();
uint256 fee = decodedTx.gasLimit * decodedTx.gasPrice;
Expand All @@ -88,7 +96,7 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
// Contract creations are signalled by sending a transaction to the zero address.
if (decodedTx.to == address(0)) {
address created = Lib_SafeExecutionManagerWrapper.safeCREATE(
decodedTx.gasLimit - 2000,
decodedTx.gasLimit,
decodedTx.data
);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
// SPDX-License-Identifier: MIT
// Pulled from @openzeppelin/contracts/math/SafeMath.sol
pragma solidity ^0.7.0;

/* Library Imports */
import { Lib_SafeExecutionManagerWrapper } from "./Lib_SafeExecutionManagerWrapper.sol";

/**
* @title Lib_SafeMathWrapper
*/

/**
* @dev Wrappers over Solidity's arithmetic operations with added overflow
* checks.
*
* Arithmetic operations in Solidity wrap on overflow. This can easily result
* in bugs, because programmers usually assume that an overflow raises an
* error, which is the standard behavior in high level programming languages.
* `SafeMath` restores this intuition by reverting the transaction when an
* operation overflows.
*
* Using this library instead of the unchecked operations eliminates an entire
* class of bugs, so it's recommended to use it always.
*/

library Lib_SafeMathWrapper {
/**
* @dev Returns the addition of two unsigned integers, reverting on
* overflow.
*
* Counterpart to Solidity's `+` operator.
*
* Requirements:
*
* - Addition cannot overflow.
*/
function add(uint256 a, uint256 b) internal returns (uint256) {
uint256 c = a + b;
Lib_SafeExecutionManagerWrapper.safeREQUIRE(c >= a, "Lib_SafeMathWrapper: addition overflow");

return c;
}

/**
* @dev Returns the subtraction of two unsigned integers, reverting on
* overflow (when the result is negative).
*
* Counterpart to Solidity's `-` operator.
*
* Requirements:
*
* - Subtraction cannot overflow.
*/
function sub(uint256 a, uint256 b) internal returns (uint256) {
return sub(a, b, "Lib_SafeMathWrapper: subtraction overflow");
}

/**
* @dev Returns the subtraction of two unsigned integers, reverting with custom message on
* overflow (when the result is negative).
*
* Counterpart to Solidity's `-` operator.
*
* Requirements:
*
* - Subtraction cannot overflow.
*/
function sub(uint256 a, uint256 b, string memory errorMessage) internal returns (uint256) {
Lib_SafeExecutionManagerWrapper.safeREQUIRE(b <= a, errorMessage);
uint256 c = a - b;

return c;
}

/**
* @dev Returns the multiplication of two unsigned integers, reverting on
* overflow.
*
* Counterpart to Solidity's `*` operator.
*
* Requirements:
*
* - Multiplication cannot overflow.
*/
function mul(uint256 a, uint256 b) internal returns (uint256) {
// Gas optimization: this is cheaper than requiring 'a' not being zero, but the
// benefit is lost if 'b' is also tested.
// See: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/522
if (a == 0) {
return 0;
}

uint256 c = a * b;
Lib_SafeExecutionManagerWrapper.safeREQUIRE(c / a == b, "Lib_SafeMathWrapper: multiplication overflow");

return c;
}

/**
* @dev Returns the integer division of two unsigned integers. Reverts on
* division by zero. The result is rounded towards zero.
*
* Counterpart to Solidity's `/` operator. Note: this function uses a
* `revert` opcode (which leaves remaining gas untouched) while Solidity
* uses an invalid opcode to revert (consuming all remaining gas).
*
* Requirements:
*
* - The divisor cannot be zero.
*/
function div(uint256 a, uint256 b) internal returns (uint256) {
return div(a, b, "Lib_SafeMathWrapper: division by zero");
}

/**
* @dev Returns the integer division of two unsigned integers. Reverts with custom message on
* division by zero. The result is rounded towards zero.
*
* Counterpart to Solidity's `/` operator. Note: this function uses a
* `revert` opcode (which leaves remaining gas untouched) while Solidity
* uses an invalid opcode to revert (consuming all remaining gas).
*
* Requirements:
*
* - The divisor cannot be zero.
*/
function div(uint256 a, uint256 b, string memory errorMessage) internal returns (uint256) {
Lib_SafeExecutionManagerWrapper.safeREQUIRE(b > 0, errorMessage);
uint256 c = a / b;
// assert(a == b * c + a % b); // There is no case in which this doesn't hold

return c;
}

/**
* @dev Returns the remainder of dividing two unsigned integers. (unsigned integer modulo),
* Reverts when dividing by zero.
*
* Counterpart to Solidity's `%` operator. This function uses a `revert`
* opcode (which leaves remaining gas untouched) while Solidity uses an
* invalid opcode to revert (consuming all remaining gas).
*
* Requirements:
*
* - The divisor cannot be zero.
*/
function mod(uint256 a, uint256 b) internal returns (uint256) {
return mod(a, b, "Lib_SafeMathWrapper: modulo by zero");
}

/**
* @dev Returns the remainder of dividing two unsigned integers. (unsigned integer modulo),
* Reverts with custom message when dividing by zero.
*
* Counterpart to Solidity's `%` operator. This function uses a `revert`
* opcode (which leaves remaining gas untouched) while Solidity uses an
* invalid opcode to revert (consuming all remaining gas).
*
* Requirements:
*
* - The divisor cannot be zero.
*/
function mod(uint256 a, uint256 b, string memory errorMessage) internal returns (uint256) {
Lib_SafeExecutionManagerWrapper.safeREQUIRE(b != 0, errorMessage);
return a % b;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,16 @@ const callPrecompile = async (
Helper_PrecompileCaller: Contract,
precompile: Contract,
functionName: string,
functionParams?: any[]
functionParams?: any[],
gasLimit?: number
): Promise<any> => {
if (gasLimit) {
return Helper_PrecompileCaller.callPrecompile(
precompile.address,
precompile.interface.encodeFunctionData(functionName, functionParams || []),
{gasLimit}
)
}
return Helper_PrecompileCaller.callPrecompile(
precompile.address,
precompile.interface.encodeFunctionData(functionName, functionParams || [])
Expand Down Expand Up @@ -177,8 +185,10 @@ describe('OVM_ECDSAContractAccount', () => {
})

it(`should revert on incorrect nonce`, async () => {
const alteredNonceTx = DEFAULT_EIP155_TX
alteredNonceTx.nonce = 99
const alteredNonceTx = {
...DEFAULT_EIP155_TX,
nonce : 99
}
const message = serializeNativeTransaction(alteredNonceTx)
const sig = await signNativeTransaction(wallet, alteredNonceTx)

Expand Down Expand Up @@ -227,5 +237,34 @@ describe('OVM_ECDSAContractAccount', () => {
'Transaction chainId does not match expected OVM chainId.'
)
})

it(`should revert on insufficient gas`, async () => {
const alteredInsufficientGasTx = {
...DEFAULT_EIP155_TX,
gasLimit : 200000000
}
const message = serializeNativeTransaction(alteredInsufficientGasTx)
const sig = await signNativeTransaction(wallet, alteredInsufficientGasTx)

await callPrecompile(
Helper_PrecompileCaller,
OVM_ECDSAContractAccount,
'execute',
[
message,
0, //isEthSignedMessage
`0x${sig.v}`, //v
`0x${sig.r}`, //r
`0x${sig.s}`, //s
],
40000000,
)

const ovmREVERT: any =
Mock__OVM_ExecutionManager.smocked.ovmREVERT.calls[0]
expect(ethers.utils.toUtf8String(ovmREVERT._data)).to.equal(
'Gas is not sufficient to execute the transaction.'
)
})
})
})

0 comments on commit 0c3c650

Please sign in to comment.