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.

Sign up for GitHub

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

Jump to bottom

fix: critical security vulnerabilities #30

Merged
merged 8 commits into from
Nov 1, 2024
Merged

fix: critical security vulnerabilities #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
98a131c
fix" changes
freemanzMrojo Nov 1, 2024
142c8f8
changes
freemanzMrojo Nov 1, 2024
ae88a55
fix
freemanzMrojo Nov 1, 2024
983eac2
some more tests fixed
freemanzMrojo Nov 1, 2024
abcca43
shard3 fixed
freemanzMrojo Nov 1, 2024
743c7fb
shard3 fixed
freemanzMrojo Nov 1, 2024
fe45433
shard3 fixed
freemanzMrojo Nov 1, 2024
adb3ab6
shard3 fixed
freemanzMrojo Nov 1, 2024
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
23 changes: 12 additions & 11 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,26 +36,31 @@
"@nomiclabs/hardhat-ethers": "^2.0.2",
"@nomiclabs/hardhat-waffle": "^2.0.1",
"@typechain/ethers-v5": "^10.1.0",
"@typechain/hardhat": "^2.3.0",
"@types/chai": "^4.2.21",
"@types/mocha": "^9.0.0",
"@types/node": "^16.4.12",
"@typescript-eslint/eslint-plugin": "^5.30.5",
"@typescript-eslint/parser": "^5.30.5",
"chai": "^4.3.4",
"chai": "^4.3.10",
"eslint": "^8.19.0",
"eslint-config-standard": "^17.0.0",
"eslint-config-standard-with-typescript": "^21.0.1",
"eslint-plugin-import": "^2.26.0",
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-promise": "^6.0.0",
"eslint-plugin-standard": "^5.0.0",
"ethereum-waffle": "^3.4.0",
"ethereum-waffle": "^4.0.10",
"ethers": "^5.4.2",
"hardhat": "^2.6.6",
"solhint": "^3.3.7",
"solidity-coverage": "^0.8.2",
"source-map-support": "^0.5.19",
"ts-generator": "^0.1.1",
"ts-mocha": "^10.0.0",
"ts-node": "^10.1.0",
"typechain": "^8.1.0"
"typechain": "^8.1.0",
"typescript": "^4.3.5"
},
"dependencies": {
"@nomicfoundation/hardhat-toolbox": "^2.0.2",
Expand All @@ -64,22 +69,18 @@
"@nomiclabs/hardhat-web3": "^2.0.0",
"@openzeppelin/contracts": "^4.9.6",
"@thehubbleproject/bls": "^0.5.1",
"@typechain/hardhat": "^2.3.0",
"@types/mocha": "^9.0.0",
"@vechain/hardhat-ethers": "^0.0.4",
"@vechain/hardhat-vechain": "^0.0.4",
"@vechain/hardhat-web3": "^0.0.4",
"@vechain/web3-providers-connex": "1.0.0",
"ethereumjs-util": "^7.1.0",
"ethereumjs-wallet": "^1.0.1",
"hardhat-deploy": "^0.11.23",
"hardhat-deploy-ethers": "^0.3.0-beta.11",
"solidity-coverage": "^0.8.2",
"source-map-support": "^0.5.19",
"table": "^6.8.0",
"typescript": "^4.3.5"
"hardhat-deploy-ethers": "^0.3.0-beta.11"
},
"resolutions": {
"@vechain/web3-providers-connex": "1.0.0"
"@vechain/web3-providers-connex": "1.0.0",
"underscore": "^1.13.7",
"ws": "^7.5.10"
}
}
11 changes: 5 additions & 6 deletions test/shard1/paymaster.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ import {
fund,
getAccountAddress,
getTokenBalance,
ONE_ETH,
rethrow
ONE_ETH
} from '../utils/testutils'
import { fillAndSign } from '../utils/UserOp'
import { UserOperation } from '../utils/UserOperation'
Expand Down Expand Up @@ -127,7 +126,7 @@ describe('EntryPoint with paymaster', function () {
}, accountOwner, entryPoint)
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
gasLimit: 1e7
})).to.revertedWith('AA33 reverted: TokenPaymaster: no balance')
})).to.revertedWith('FailedOp').withArgs(0, 'AA33 reverted: TokenPaymaster: no balance')

// This reverts as expected but its not reflected in the test case
// await expect(entryPoint.handleOps([op], beneficiaryAddress, {
Expand All @@ -149,7 +148,7 @@ describe('EntryPoint with paymaster', function () {
}, accountOwner, entryPoint)
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
gasLimit: 1e7
}).catch(rethrow())).to.revertedWith('TokenPaymaster: no balance')
})).to.revertedWith('FailedOp').withArgs(0, 'AA33 reverted: TokenPaymaster: no balance (pre-create)')
})

it('should succeed to create account with tokens', async () => {
Expand Down Expand Up @@ -193,7 +192,7 @@ describe('EntryPoint with paymaster', function () {
if (!created) this.skip()
await expect(entryPoint.callStatic.handleOps([createOp], beneficiaryAddress, {
gasLimit: 1e7
}).catch(rethrow())).to.revertedWith('sender already constructed')
})).to.revertedWith('FailedOp').withArgs(0, 'AA10 sender already constructed')
})

it('batched request should each pay for its share', async function () {
Expand Down Expand Up @@ -316,7 +315,7 @@ describe('EntryPoint with paymaster', function () {
this.timeout(20000)
await expect(
paymaster.withdrawStake(withdrawAddress)
).to.revertedWith('must call unlockStake')
).to.revertedWith('must call unlockStake() first')
})
it('should be able to withdraw after unstake delay', async () => {
await paymaster.unlockStake()
Expand Down
12 changes: 6 additions & 6 deletions test/shard2/entrypoint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ describe('EntryPoint', function () {
// using wrong nonce
const op = await fillAndSign({ sender: account.address, nonce: 1234 }, accountOwner, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op)).to
.revertedWith('AA25 invalid account nonce')
.revertedWith('FailedOp').withArgs(0, 'AA25 invalid account nonce')
})

it('should report signature failure without revert', async () => {
Expand All @@ -362,13 +362,13 @@ describe('EntryPoint', function () {
verificationGasLimit: 1000
}, accountOwner, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op)).to
.revertedWith('AA20 account not deployed')
.revertedWith('FailedOp').withArgs(0, 'AA20 account not deployed')
})

it('should revert on oog if not enough verificationGas', async () => {
const op = await fillAndSign({ sender: account.address, verificationGasLimit: 1000 }, accountOwner, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op)).to
.revertedWith('AA23 reverted (or OOG)')
.revertedWith('FailedOp').withArgs(0, 'AA23 reverted (or OOG)')
})

it('should succeed if validateUserOp succeeds', async () => {
Expand Down Expand Up @@ -435,7 +435,7 @@ describe('EntryPoint', function () {
}, accountOwner1, entryPoint)
await expect(
entryPoint.callStatic.simulateValidation(op)
).to.revertedWith('gas values overflow')
).to.revertedWith('AA94 gas values overflow')
})

it('should fail creation for wrong sender', async () => {
Expand All @@ -445,7 +445,7 @@ describe('EntryPoint', function () {
verificationGasLimit: 3e6
}, accountOwner1, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op1))
.to.revertedWith('AA14 initCode must return sender')
.to.revertedWith('FailedOp').withArgs(0, 'AA14 initCode must return sender')
})

it('should report failure on insufficient verificationGas (OOG) for creation', async () => {
Expand All @@ -469,7 +469,7 @@ describe('EntryPoint', function () {
maxFeePerGas: 0
}, accountOwner1, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op1, { gasLimit: 1e6 }))
.to.revertedWith('AA13 initCode failed or OOG')
.to.revertedWith('FailedOp').withArgs(0, 'AA13 initCode failed or OOG')
})

it('should succeed for creating an account', async () => {
Expand Down
30 changes: 15 additions & 15 deletions test/shard3/entrypoint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ describe('EntryPoint', function () {
sender,
nonce: keyShifted.add(1)
}, accountOwner, entryPoint)
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('AA25 invalid account nonce')
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('FailedOp').withArgs(0, 'AA25 invalid account nonce')
})

describe('with key=1, seq=1', () => {
Expand Down Expand Up @@ -335,7 +335,7 @@ describe('EntryPoint', function () {
sender,
nonce: keyShifted.add(3)
}, accountOwner, entryPoint)
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('AA25 invalid account nonce')
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('FailedOp').withArgs(0, 'AA25 invalid account nonce')
})
})
})
Expand Down Expand Up @@ -373,7 +373,7 @@ describe('EntryPoint', function () {
sender: account.address
}, wrongOwner, entryPoint)
const beneficiaryAddress = createAddress()
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('AA24 signature error')
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('FailedOp').withArgs(0, 'AA24 signature error')
})

it('account should pay for tx', async function () {
Expand Down Expand Up @@ -469,7 +469,7 @@ describe('EntryPoint', function () {
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
maxFeePerGas: 1e9,
gasLimit: 12e5
})).to.revertedWith('AA95 out of gas')
})).to.revertedWith('FailedOp').withArgs(0, 'AA95 out of gas')

// Make sure that the user did not pay for the transaction
expect(await getBalance(account.address)).to.eq(inititalAccountBalance)
Expand Down Expand Up @@ -622,7 +622,7 @@ describe('EntryPoint', function () {
verificationGasLimit: 1000
}, accountOwner, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op1))
.to.revertedWith('AA23 reverted (or OOG)')
.to.revertedWith('FailedOp').withArgs(0, 'AA23 reverted (or OOG)')
})
})

Expand All @@ -642,7 +642,7 @@ describe('EntryPoint', function () {

await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
gasLimit: 1e7
})).to.revertedWith('AA14 initCode must return sender')
})).to.revertedWith('FailedOp').withArgs(0, 'AA14 initCode must return sender')
})

it('should reject create if account not funded', async () => {
Expand All @@ -656,7 +656,7 @@ describe('EntryPoint', function () {
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress, {
gasLimit: 1e7,
gasPrice: await ethers.provider.getGasPrice()
})).to.revertedWith('didn\'t pay prefund')
})).to.revertedWith('FailedOp').withArgs(0, 'AA21 didn\'t pay prefund')

// await expect(await ethers.provider.getCode(op.sender).then(x => x.length)).to.equal(2, "account exists before creation")
})
Expand Down Expand Up @@ -697,7 +697,7 @@ describe('EntryPoint', function () {

await expect(entryPoint.callStatic.handleOps([createOp], beneficiaryAddress, {
gasLimit: 1e7
})).to.revertedWith('sender already constructed')
})).to.revertedWith('FailedOp').withArgs(0, 'AA10 sender already constructed')
})
})

Expand Down Expand Up @@ -797,7 +797,7 @@ describe('EntryPoint', function () {
}, accountOwner, entryPoint)

// no aggregator is kind of "wrong aggregator"
await expect(entryPoint.callStatic.handleOps([userOp], beneficiaryAddress)).to.revertedWith('AA24 signature error')
await expect(entryPoint.callStatic.handleOps([userOp], beneficiaryAddress)).to.revertedWith('FailedOp').withArgs(0, 'AA24 signature error')
})
it('should fail to execute aggregated account with wrong aggregator', async () => {
const userOp = await fillAndSign({
Expand All @@ -811,7 +811,7 @@ describe('EntryPoint', function () {
userOps: [userOp],
aggregator: wrongAggregator.address,
signature: sig
}], beneficiaryAddress)).to.revertedWith('AA24 signature error')
}], beneficiaryAddress)).to.revertedWith('FailedOp').withArgs(0, 'AA24 signature error')
})

it('should reject non-contract (address(1)) aggregator', async () => {
Expand Down Expand Up @@ -993,7 +993,7 @@ describe('EntryPoint', function () {
verificationGasLimit: 3e6,
callGasLimit: 1e6
}, account2Owner, entryPoint)
await expect(entryPoint.callStatic.simulateValidation(op)).to.revertedWith('"AA30 paymaster not deployed"')
await expect(entryPoint.callStatic.simulateValidation(op)).to.revertedWith('FailedOp').withArgs(0, 'AA30 paymaster not deployed')
})

it('should fail if paymaster has no deposit', async function () {
Expand All @@ -1006,7 +1006,7 @@ describe('EntryPoint', function () {
callGasLimit: 1e6
}, account2Owner, entryPoint)
const beneficiaryAddress = createAddress()
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('"AA31 paymaster deposit too low"')
await expect(entryPoint.callStatic.handleOps([op], beneficiaryAddress)).to.revertedWith('FailedOp').withArgs(0, 'AA31 paymaster deposit too low')
})

it('paymaster should pay for tx', async function () {
Expand Down Expand Up @@ -1183,7 +1183,7 @@ describe('EntryPoint', function () {
it('handleOps should revert on expired paymaster request', async () => {
const userOp = await createOpWithPaymasterParams(sessionOwner, now + 100, now + 200)
await expect(entryPoint.callStatic.handleOps([userOp], beneficiary))
.to.revertedWith('AA22 expired or not due')
.to.revertedWith('FailedOp').withArgs(0, 'AA22 expired or not due')
})
})
})
Expand All @@ -1198,7 +1198,7 @@ describe('EntryPoint', function () {
sender: account.address
}, expiredOwner, entryPoint)
await expect(entryPoint.callStatic.handleOps([userOp], beneficiary))
.to.revertedWith('AA22 expired or not due')
.to.revertedWith('FailedOp').withArgs(0, 'AA22 expired or not due')
})

// this test passed when running it individually but fails when its run alonside the other tests
Expand All @@ -1211,7 +1211,7 @@ describe('EntryPoint', function () {
sender: account.address
}, futureOwner, entryPoint)
await expect(entryPoint.callStatic.handleOps([userOp], beneficiary))
.to.revertedWith('AA22 expired or not due')
.to.revertedWith('FailedOp').withArgs(0, 'AA22 expired or not due')
})
})
})
Expand Down
Loading
Loading