Skip to content

N
nebula
Commit 8a10f31b authored by Kelvin Fichter's avatar Kelvin Fichter
Browse files
Options

Added XOR fix

parent c7921565
Hide whitespace changes
Inline Side-by-side
Showing with 67 additions and 67 deletions
packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
View file @ 8a10f31b
......@@ -79,14 +79,14 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager {
modifier netGasCost(
uint256 _cost
) {
uint256 preExecutionGas = gasleft();
uint256 gasProvided = gasleft();
_;
uint256 postExecutionGas = gasleft();
uint256 gasUsed = gasProvided - gasleft();
// We want to refund everything *except* the specified cost.
transactionRecord.ovmGasRefund += (
(preExecutionGas - postExecutionGas) - _cost
);
if (_cost < gasUsed) {
transactionRecord.ovmGasRefund += gasUsed - _cost;
}
}
/**
......@@ -133,13 +133,13 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager {
_initContext(_transaction);
// Run the transaction, make sure to meter the gas usage.
uint256 gasLimit = gasleft();
uint256 gasProvided = gasleft();
ovmCALL(
_transaction.gasLimit - gasMeterConfig.minTransactionGasLimit,
_transaction.entrypoint,
_transaction.data
);
uint256 gasUsed = gasLimit - gasleft();
uint256 gasUsed = gasProvided - gasleft();
// Update the cumulative gas based on the amount of gas used.
_updateCumulativeGas(gasUsed, _transaction.queueOrigin);
......@@ -1126,11 +1126,15 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager {
)
internal
{
// We need to make sure that the transaction isn't trying to access an account that hasn't
// been provided to the OVM_StateManager. We'll immediately abort if this is the case.
_checkInvalidStateAccess(
ovmStateManager.hasAccount(_address)
);
// See `_checkContractStorageLoad` for more information.
if (gasleft() < MIN_GAS_FOR_INVALID_STATE_ACCESS) {
_revertWithFlag(RevertFlag.OUT_OF_GAS);
}
// See `_checkContractStorageLoad` for more information.
if (ovmStateManager.hasAccount(_address) == false) {
_revertWithFlag(RevertFlag.INVALID_STATE_ACCESS);
}
// Check whether the account has been loaded before and mark it as loaded if not. We need
// this because "nuisance gas" only applies to the first time that an account is loaded.
......@@ -1185,11 +1189,22 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager {
)
internal
{
// Another case of hidden complexity. If we didn't enforce this requirement, then a
// contract could pass in just enough gas to cause the INVALID_STATE_ACCESS check to fail
// on L1 but not on L2. A contract could use this behavior to prevent the
// OVM_ExecutionManager from detecting an invalid state access. Reverting with OUT_OF_GAS
// allows us to also charge for the full message nuisance gas, because you deserve that for
// trying to break the contract in this way.
if (gasleft() < MIN_GAS_FOR_INVALID_STATE_ACCESS) {
_revertWithFlag(RevertFlag.OUT_OF_GAS);
}
// We need to make sure that the transaction isn't trying to access storage that hasn't
// been provided to the OVM_StateManager. We'll immediately abort if this is the case.
_checkInvalidStateAccess(
ovmStateManager.hasContractStorage(_contract, _key)
);
// We know that we have enough gas to do this check because of the above test.
if (ovmStateManager.hasContractStorage(_contract, _key) == false) {
_revertWithFlag(RevertFlag.INVALID_STATE_ACCESS);
}
// Check whether the slot has been loaded before and mark it as loaded if not. We need
// this because "nuisance gas" only applies to the first time that a slot is loaded.
......@@ -1216,11 +1231,15 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager {
)
internal
{
// We need to make sure that the transaction isn't trying to access storage that hasn't
// been provided to the OVM_StateManager. We'll immediately abort if this is the case.
_checkInvalidStateAccess(
ovmStateManager.hasContractStorage(_contract, _key)
);
// See `_checkContractStorageLoad` for more information.
if (gasleft() < MIN_GAS_FOR_INVALID_STATE_ACCESS) {
_revertWithFlag(RevertFlag.OUT_OF_GAS);
}
// See `_checkContractStorageLoad` for more information.
if (ovmStateManager.hasContractStorage(_contract, _key) == false) {
_revertWithFlag(RevertFlag.INVALID_STATE_ACCESS);
}
// Check whether the slot has been changed before and mark it as changed if not. We need
// this because "nuisance gas" only applies to the first time that a slot is changed.
......@@ -1361,30 +1380,6 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager {
_revertWithFlag(_flag, bytes(''));
}
/**
* Checks for an attempt to access some inaccessible state.
* @param _condition Result of some function that checks for bad access.
*/
function _checkInvalidStateAccess(
bool _condition
)
internal
{
// Another case of hidden complexity. If we didn't enforce this requirement, then a
// contract could pass in just enough gas to cause this to fail on L1 but not on L2.
// A contract could use this behavior to prevent the OVM_ExecutionManager from detecting
// an invalid state access. Reverting with OUT_OF_GAS allows us to also charge for the
// full message nuisance gas as to generally disincentivize this attack.
if (gasleft() < MIN_GAS_FOR_INVALID_STATE_ACCESS) {
_revertWithFlag(RevertFlag.OUT_OF_GAS);
}
// We have enough gas to comfortably run this revert, so do it.
if (_condition == false) {
_revertWithFlag(RevertFlag.INVALID_STATE_ACCESS);
}
}
/******************************************
* Internal Functions: Nuisance Gas Logic *
......
packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_StateManager.sol
View file @ 8a10f31b
......@@ -18,6 +18,7 @@ contract OVM_StateManager is iOVM_StateManager {
**********************/
bytes32 constant internal EMPTY_ACCOUNT_CODE_HASH = 0x00004B1DC0DE000000004B1DC0DE000000004B1DC0DE000000004B1DC0DE0000;
bytes32 constant internal STORAGE_XOR_VALUE = 0xFEEDFACECAFEBEEFFEEDFACECAFEBEEFFEEDFACECAFEBEEFFEEDFACECAFEBEEF;
/*******************************************
......@@ -365,7 +366,10 @@ contract OVM_StateManager is iOVM_StateManager {
public
authenticated
{
contractStorage[_contract][_key] = _value;
// A hilarious optimization. `SSTORE`ing a value of `bytes32(0)` is common enough that it's
// worth populating this with a non-zero value in advance (during the fraud proof
// initialization phase) to cut the execution-time cost down to 5000 gas.
contractStorage[_contract][_key] = _value ^ STORAGE_XOR_VALUE;
// Only used when initially populating the contract storage. OVM_ExecutionManager will
// perform a `hasContractStorage` INVALID_STATE_ACCESS check before putting any contract
......@@ -397,7 +401,8 @@ contract OVM_StateManager is iOVM_StateManager {
bytes32 _value
)
{
return contractStorage[_contract][_key];
// See `putContractStorage` for more information about the XOR here.
return contractStorage[_contract][_key] ^ STORAGE_XOR_VALUE;
}
/**
......
packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager/run.spec.ts
View file @ 8a10f31b
......@@ -18,7 +18,7 @@ enum GasMetadataKey {
CUMULATIVE_SEQUENCER_QUEUE_GAS,
CUMULATIVE_L1TOL2_QUEUE_GAS,
PREV_EPOCH_SEQUENCER_QUEUE_GAS,
PREV_EPOCH_L1TOL2_QUEUE_GAS
PREV_EPOCH_L1TOL2_QUEUE_GAS,
}
const keyToBytes32 = (key: GasMetadataKey): string => {
......@@ -57,8 +57,8 @@ const test_run: TestDefinition = {
[keyToBytes32(GasMetadataKey.CUMULATIVE_SEQUENCER_QUEUE_GAS)]: 0,
[keyToBytes32(GasMetadataKey.CUMULATIVE_L1TOL2_QUEUE_GAS)]: 0,
[keyToBytes32(GasMetadataKey.PREV_EPOCH_SEQUENCER_QUEUE_GAS)]: 0,
[keyToBytes32(GasMetadataKey.PREV_EPOCH_L1TOL2_QUEUE_GAS)]: 0
}
[keyToBytes32(GasMetadataKey.PREV_EPOCH_L1TOL2_QUEUE_GAS)]: 0,
},
},
verifiedContractStorage: {
[GAS_METADATA_ADDRESS]: {
......@@ -67,8 +67,8 @@ const test_run: TestDefinition = {
[keyToBytes32(GasMetadataKey.CUMULATIVE_L1TOL2_QUEUE_GAS)]: true,
[keyToBytes32(GasMetadataKey.PREV_EPOCH_SEQUENCER_QUEUE_GAS)]: true,
[keyToBytes32(GasMetadataKey.PREV_EPOCH_L1TOL2_QUEUE_GAS)]: true,
}
}
},
},
},
},
parameters: [
......@@ -100,12 +100,12 @@ const test_run: TestDefinition = {
},
expectedReturnStatus: true,
},
]
}
}
],
},
},
],
},
]
],
}
const runner = new ExecutionManagerTestRunner()
......
packages/contracts/test/helpers/storage/contract-storage.ts
View file @ 8a10f31b
......@@ -178,14 +178,14 @@ export const setContractStorage = async (
const baseSlotHash = getStorageSlotHash(slot, depth, {
[subKey1]: {
[subKey]: subValue,
}
},
})
const slotValues = getFlattenedValues(depth, {
[subKey1]: {
[subKey]: subValue,
}
},
})
for (const slotValue of slotValues) {
const slotIndex = inputSlots.find((inputSlot) => {
return inputSlot.label === slotValue.label
......@@ -193,7 +193,7 @@ export const setContractStorage = async (
const slotHash = toHexString32(
BigNumber.from(baseSlotHash).add(slotIndex)
)
await contract.__setStorageSlot(slotHash, slotValue.value)
}
}
......@@ -257,14 +257,14 @@ export const checkContractStorage = async (
const baseSlotHash = getStorageSlotHash(slot, depth, {
[subKey1]: {
[subKey]: subValue,
}
},
})
const slotValues = getFlattenedValues(depth, {
[subKey1]: {
[subKey]: subValue,
}
},
})
for (const slotValue of slotValues) {
const slotIndex = inputSlots.find((inputSlot) => {
return inputSlot.label === slotValue.label
......@@ -274,7 +274,7 @@ export const checkContractStorage = async (
)
const retSlotValue = await contract.__getStorageSlot(slotHash)
if (retSlotValue !== slotValue.value) {
throw new Error(
`Resulting state of ${slotValue.label} (${retSlotValue}) did not match expected state (${slotValue.value}).`
......
packages/contracts/test/helpers/test-utils/test-runner.ts
View file @ 8a10f31b
......@@ -204,9 +204,9 @@ export class ExecutionManagerTestRunner {
functionParams: {
gasLimit: GAS_LIMIT,
target: this.contracts.Helper_TestRunner.address,
subSteps: step.functionParams.subSteps
subSteps: step.functionParams.subSteps,
},
expectedReturnStatus: true
expectedReturnStatus: true,
}
calldata = this.encodeFunctionData(runStep)
......@@ -220,7 +220,7 @@ export class ExecutionManagerTestRunner {
origin: step.functionParams.origin,
msgSender: step.functionParams.msgSender,
gasLimit: step.functionParams.gasLimit,
data: calldata
data: calldata,
},
this.contracts.OVM_StateManager.address
)
......
packages/contracts/test/helpers/test-utils/test.types.ts
View file @ 8a10f31b
......@@ -117,7 +117,7 @@ interface TestStep_CREATE2 {
}
export interface TestStep_Run {
functionName: 'run',
functionName: 'run'
functionParams: {
timestamp: number
queueOrigin: number
......
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