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

tx/vm/util: throw when provided negative BN #1606

Merged
merged 24 commits into from
Dec 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b109b39
tx: throw on negative bn txdata
gabrocheleau Dec 12, 2021
fc29427
util: remove negative bn support in toBuffer method
gabrocheleau Dec 12, 2021
5c69fc7
vm: throw on negative message value field
gabrocheleau Dec 12, 2021
5741479
vm: fix boolean check
gabrocheleau Dec 12, 2021
a006183
vm: add runCall test for negative BN value
gabrocheleau Dec 12, 2021
7061f77
vm: more specific runCall test
gabrocheleau Dec 12, 2021
7c349ba
vm: address ryan's review
gabrocheleau Dec 12, 2021
aa75c16
Merge branch 'master' into vm/tx/remove-negative-bn-support
gabrocheleau Dec 15, 2021
7c21516
chore: can not -> cannot and undo reordering
gabrocheleau Dec 15, 2021
49f9196
vm: runCode test case for negative value
gabrocheleau Dec 15, 2021
2ca55dc
Merge branch 'master' into vm/tx/remove-negative-bn-support
jochem-brouwer Dec 15, 2021
bb67bc5
vm: add runTx negative value test
gabrocheleau Dec 15, 2021
8bada8a
vm: remove lint no console override
gabrocheleau Dec 15, 2021
c44c555
Update packages/vm/tests/api/runTx.spec.ts
gabrocheleau Dec 15, 2021
d8496ed
vm: remove console log
gabrocheleau Dec 15, 2021
726e0b7
Merge branch 'vm/tx/remove-negative-bn-support' of https://github.com…
gabrocheleau Dec 15, 2021
9b04c11
vm: remove console logs
gabrocheleau Dec 15, 2021
a1c98f9
vm: eslintrc exception instead of per-file
gabrocheleau Dec 15, 2021
1508db8
Update packages/vm/tests/api/runCall.spec.ts
gabrocheleau Dec 15, 2021
b15319f
vm: address review
gabrocheleau Dec 15, 2021
cb934f4
Merge branch 'vm/tx/remove-negative-bn-support' of https://github.com…
gabrocheleau Dec 15, 2021
834b991
Update packages/vm/tests/api/runCode.spec.ts
gabrocheleau Dec 15, 2021
151f6a9
Update packages/vm/tests/api/runTx.spec.ts
gabrocheleau Dec 15, 2021
420328c
Update packages/vm/tests/api/runCall.spec.ts
gabrocheleau Dec 15, 2021
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
4 changes: 3 additions & 1 deletion packages/tx/test/eip1559.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const validSlot = Buffer.from('01'.repeat(32), 'hex')
const chainId = new BN(4)

tape('[FeeMarketEIP1559Transaction]', function (t) {
t.test('cannot input decimal values', (st) => {
t.test('cannot input decimal or negative values', (st) => {
const values = [
'maxFeePerGas',
'maxPriorityFeePerGas',
Expand All @@ -33,6 +33,8 @@ tape('[FeeMarketEIP1559Transaction]', function (t) {
'0xaa.1',
-10.1,
-1,
new BN(-10),
'-100',
'-10.1',
'-0xaa',
Infinity,
Expand Down
4 changes: 3 additions & 1 deletion packages/tx/test/legacy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ const txFixturesEip155: VitaliksTestsDataEntry[] = require('./json/ttTransaction
tape('[Transaction]', function (t) {
const transactions: Transaction[] = []

t.test('cannot input decimal values', (st) => {
t.test('cannot input decimal or negative values', (st) => {
const values = ['gasPrice', 'gasLimit', 'nonce', 'value', 'v', 'r', 's']
const cases = [
10.1,
'10.1',
'0xaa.1',
-10.1,
-1,
new BN(-10),
'-100',
'-10.1',
'-0xaa',
Infinity,
Expand Down
5 changes: 5 additions & 0 deletions packages/tx/test/transactionFactory.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,11 @@ tape('[TransactionFactory]: Basic functions', function (t) {
st.throws(() => {
TransactionFactory.fromTxData({ type: 999 })
})

st.throws(() => {
TransactionFactory.fromTxData({ value: new BN('-100') })
})

st.end()
})

Expand Down
2 changes: 2 additions & 0 deletions packages/tx/test/typedTxsAndEIP2930.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ tape(
'0xaa.1',
-10.1,
-1,
new BN(-10),
'-100',
'-10.1',
'-0xaa',
Infinity,
Expand Down
3 changes: 3 additions & 0 deletions packages/util/src/bytes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@ export const toBuffer = function (v: ToBufferInputTypes): Buffer {
}

if (BN.isBN(v)) {
if (v.isNeg()) {
throw new Error(`Cannot convert negative BN to buffer. Given: ${v}`)
}
return v.toArrayLike(Buffer)
}

Expand Down
3 changes: 3 additions & 0 deletions packages/util/test/bytes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,9 @@ tape('toBuffer', function (t) {
st.throws(function () {
toBuffer({ test: 1 } as any)
})
st.throws(function () {
toBuffer(new BN(-10))
})
st.end()
})

Expand Down
34 changes: 17 additions & 17 deletions packages/vm/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
module.exports = {
extends: '../../config/eslint.js',
ignorePatterns: ['scripts', 'benchmarks', 'examples', 'karma.conf.js'],
rules: {
'@typescript-eslint/no-use-before-define': 'off',
'no-invalid-this': 'off',
'no-restricted-syntax': 'off',
},
overrides: [
{
files: ['tests/**/*.ts'],
rules: {
'no-console': 'off',
},
},
],
}
module.exports = {
extends: '../../config/eslint.js',
ignorePatterns: ['scripts', 'benchmarks', 'examples', 'karma.conf.js'],
rules: {
'@typescript-eslint/no-use-before-define': 'off',
'no-invalid-this': 'off',
'no-restricted-syntax': 'off',
},
overrides: [
{
files: ['tests/util.ts', 'tests/tester/**/*.ts'],
rules: {
'no-console': 'off',
},
},
],
}
4 changes: 4 additions & 0 deletions packages/vm/src/evm/message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ export default class Message {
this.salt = opts.salt // For CREATE2, TODO: Move from here
this.selfdestruct = opts.selfdestruct // TODO: Move from here
this.delegatecall = opts.delegatecall || false

if (this.value.isNeg()) {
throw new Error(`value field cannot be negative, received ${this.value}`)
}
}

get codeAddress(): Address {
Expand Down
4 changes: 2 additions & 2 deletions packages/vm/src/evm/opcodes/EIP2929.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export function accessAddressEIP2929(
// Cold
if (!(<EIP2929StateManager>runState.stateManager).isWarmedAddress(addressStr)) {
// eslint-disable-next-line prettier/prettier
(<EIP2929StateManager>runState.stateManager).addWarmedAddress(addressStr)
(<EIP2929StateManager>runState.stateManager).addWarmedAddress(addressStr)

// CREATE, CREATE2 opcodes have the address warmed for free.
// selfdestruct beneficiary address reads are charged an *additional* cold access
Expand Down Expand Up @@ -69,7 +69,7 @@ export function accessStorageEIP2929(
// Cold (SLOAD and SSTORE)
if (slotIsCold) {
// eslint-disable-next-line prettier/prettier
(<EIP2929StateManager>runState.stateManager).addWarmedStorage(address, key)
(<EIP2929StateManager>runState.stateManager).addWarmedStorage(address, key)
runState.eei.useGas(new BN(common.param('gasPrices', 'coldsload')), 'EIP-2929 -> coldsload')
} else if (!isSstore) {
runState.eei.useGas(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ tape('EIP-2565 ModExp gas cost tests', (t) => {
}

if (!result.execResult.returnValue.equals(Buffer.from(test.Expected, 'hex'))) {
console.log(result.execResult.returnValue.toString('hex'))
st.fail(`[${testName}]: Return value not the expected value`)
continue
}
Expand Down
2 changes: 0 additions & 2 deletions packages/vm/tests/api/EIPs/eip-3541.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ tape('EIP 3541 tests', (t) => {
nonce: 1,
}).sign(pkey)

console.log('tx1')

await vm.runTx({ tx: tx1 })

code = await vm.stateManager.getContractCode(address!)
Expand Down
25 changes: 22 additions & 3 deletions packages/vm/tests/api/runCall.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,8 @@ tape('Ensure that Istanbul sstoreCleanRefundEIP2200 gas is applied correctly', a
}

const result = await vm.runCall(runCallArgs)
console.log(result.gasUsed, result.execResult.gasRefund)
t.equal(result.gasUsed.toNumber(), 5812, 'gas used incorrect')
t.equal(result.execResult.gasRefund!.toNumber(), 4200, 'gas refund incorrect')
t.ok(result.gasUsed.eqn(5812), 'gas used correct')
t.ok(result.execResult.gasRefund!.eqn(4200), 'gas refund correct')

t.end()
})
Expand Down Expand Up @@ -318,3 +317,23 @@ tape('Ensure that IDENTITY precompile copies the memory', async (t) => {

t.end()
})

tape('Throws on negative call value', async (t) => {
ryanio marked this conversation as resolved.
Show resolved Hide resolved
// setup the vm
const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Istanbul })
const vm = new VM({ common: common })

// setup the call arguments
const runCallArgs = {
value: new BN(-10),
}

try {
await vm.runCall(runCallArgs)
t.fail('should not accept a negative call value')
} catch (err: any) {
t.ok(err.message.includes('value field cannot be negative'), 'throws on negative call value')
}

t.end()
})
19 changes: 19 additions & 0 deletions packages/vm/tests/api/runCode.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,22 @@ tape('VM.runCode: interpreter', (t) => {
st.end()
})
})

tape('VM.runCode: RunCodeOptions', (t) => {
t.test('should throw on negative value args', async (st) => {
const vm = new VM()

const runCodeArgs = {
value: new BN('-10'),
}

try {
await vm.runCode(runCodeArgs)
st.fail('should not accept a negative call value')
} catch (err: any) {
st.ok(err.message.includes('value field cannot be negative'), 'throws on negative call value')
}

st.end()
})
})
27 changes: 26 additions & 1 deletion packages/vm/tests/api/runTx.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const TRANSACTION_TYPES = [
name: 'EIP1559 tx',
},
]

const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.London })
common.setMaxListeners(100)

Expand Down Expand Up @@ -328,7 +329,7 @@ tape('runTx() -> runtime behavior', async (t) => {
'e331b6d69882b4cb4ea581d88e0b604039a3de5967688d3dcffdd2270c0fd109',
'hex'
)
/* Code which is deployed here:
/* Code which is deployed here:
PUSH1 01
PUSH1 00
SSTORE
Expand Down Expand Up @@ -559,3 +560,27 @@ tape('runTx() -> consensus bugs', async (t) => {
t.end()
})
})

tape('runTx() -> RunTxOptions', (t) => {
t.test('should throw on negative value args', async (t) => {
const vm = new VM({ common })
for (const txType of TRANSACTION_TYPES) {
const tx = getTransaction(vm._common, txType.type, true)
tx.value.isubn(1)

try {
await vm.runTx({
tx,
skipBalance: true,
})
t.fail('should not accept a negative call value')
} catch (err: any) {
t.ok(
err.message.includes('value field cannot be negative'),
'throws on negative call value'
)
}
}
t.end()
})
})
1 change: 0 additions & 1 deletion packages/vm/tests/tester/runners/BlockchainTestsRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ export default async function runBlockchainTest(options: any, testData: any, t:
if (expectException) {
t.pass(`Expected exception ${expectException}`)
} else {
console.log(error)
t.fail(error)
}
}
Expand Down