Skip to content

Commit

Permalink
fix: critical security vulnerabilities (#30)
Browse files Browse the repository at this point in the history 
* fix" changes

* changes

* fix

* some more tests fixed

* shard3 fixed

* shard3 fixed

* shard3 fixed

* shard3 fixed
  • Loading branch information
@freemanzMrojo
freemanzMrojo authored Nov 1, 2024
1 parent 76b1622 commit e90d6e1
Show file tree
Hide file tree
Showing 5 changed files with 628 additions and 3,360 deletions.
23 changes: 12 additions & 11 deletions package.json
View file
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
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
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
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

0 comments on commit e90d6e1

Please sign in to comment.