Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

Bw 458 audit fix 06 #604

Merged
merged 5 commits into from
Jun 27, 2023
Merged

Bw 458 audit fix 06 #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50b957a
WIP AI6 - test fail
jzaki Apr 5, 2023
e4c7dfb
Revert "WIP AI6 - test fail"
jzaki May 22, 2023
60effa1
Resolve AI6 by removing wallet upgradability
jzaki May 22, 2023
9a9b028
Fix linter errors
jzaki May 22, 2023
ada8500
Merge branch 'contract-updates' of github.com:web3well/bls-wallet int…
jacque006 Jun 27, 2023
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
8 changes: 6 additions & 2 deletions contracts/contracts/VerificationGateway.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,7 @@ contract VerificationGateway
IWallet wallet = IWallet(msg.sender);
bytes32 existingHash = hashFromWallet[wallet];
if (existingHash == bytes32(0)) { // wallet does not yet have a bls key registered with this gateway
// set it instantly
safeSetWallet(messageSenderSignature, publicKey, wallet, signatureExpiryTimestamp);
// Can't register new wallet contracts, only what this gateway deployed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire logically block would be easier to follow as a single if statement, since the original branch is no a no-op.
(couldn't use suggest changes)

...
// Can't register new wallet contracts, only what this gateway deployed.
if (existingHash != bytes32(0)) { // wallet already has a key registered, set after delay
    pendingMessageSenderSignatureFromHash[existingHash] = messageSenderSignature;
    pendingBLSPublicKeyFromHash[existingHash] = publicKey;
    pendingBLSPublicKeyTimeFromHash[existingHash] = block.timestamp + 604800; // 1 week from now
    emit PendingBLSKeySet(existingHash, publicKey);
}
...

}
else { // wallet already has a key registered, set after delay
pendingMessageSenderSignatureFromHash[existingHash] = messageSenderSignature;
Expand Down Expand Up @@ -235,6 +234,11 @@ contract VerificationGateway
}
}

require((selectorId != ProxyAdmin.upgrade.selector)
&& (selectorId != ProxyAdmin.upgradeAndCall.selector),
"VG: wallet not upgradable"
);

wallet.setAnyPending();

// ensure wallet has pre-approved encodedFunction
Expand Down
250 changes: 9 additions & 241 deletions contracts/test/upgrade-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,13 @@
import { expect } from "chai";
import { BigNumber, ContractReceipt } from "ethers";
import { solidityPack } from "ethers/lib/utils";
import { ethers, network } from "hardhat";
import { ethers } from "hardhat";

import { expectPubkeysEql } from "./expect";
import {
ActionData,
BlsWalletWrapper,
getOperationResults,
} from "../clients/src";
import { ActionData, getOperationResults } from "../clients/src";
import Fixture from "../shared/helpers/Fixture";
import {
proxyAdminBundle,
proxyAdminCall,
} from "../shared/helpers/callProxyAdmin";
import { proxyAdminCall } from "../shared/helpers/callProxyAdmin";
import getPublicKeyFromHash from "../shared/helpers/getPublicKeyFromHash";
import deploy from "../shared/deploy";

const expectOperationsToSucceed = (txnReceipt: ContractReceipt) => {
const opResults = getOperationResults(txnReceipt);
Expand Down Expand Up @@ -44,7 +36,7 @@ describe("Upgrade", async function () {
fx = await Fixture.getSingleton();
});

it("should upgrade wallet contract", async () => {
it("should NOT upgrade wallet contract", async () => {
const MockWalletUpgraded = await ethers.getContractFactory(
"MockWalletUpgraded",
);
Expand All @@ -57,243 +49,19 @@ describe("Upgrade", async function () {
wallet.address,
mockWalletUpgraded.address,
]);
expectOperationsToSucceed(txnReceipt1);

// Advance time one week
const latestTimestamp = (await ethers.provider.getBlock("latest"))
.timestamp;
await network.provider.send("evm_setNextBlockTimestamp", [
BigNumber.from(latestTimestamp)
.add(safetyDelaySeconds + 1)
.toHexString(),
]);
expectOperationFailure(txnReceipt1, "VG: wallet not upgradable");

// make call
const txnReceipt2 = await proxyAdminCall(fx, wallet, "upgrade", [
const txnReceipt2 = await proxyAdminCall(fx, wallet, "upgradeAndCall", [
wallet.address,
mockWalletUpgraded.address,
[],
]);
expectOperationsToSucceed(txnReceipt2);

const newBLSWallet = MockWalletUpgraded.attach(wallet.address);
await (await newBLSWallet.setNewData(wallet.address)).wait();
await expect(newBLSWallet.newData()).to.eventually.equal(wallet.address);
expectOperationFailure(txnReceipt2, "VG: wallet not upgradable");
});

it("should register with new verification gateway", async () => {
// Deploy new verification gateway

const [signer] = await ethers.getSigners();

const deployment2 = await deploy(
signer,
ethers.utils.solidityPack(["uint256"], [2]),
);

const vg2 = deployment2.verificationGateway;

// Recreate hubble bls signer
const walletOldVg = await fx.createBLSWallet();
const walletAddress = walletOldVg.address;
const blsSecret = walletOldVg.blsWalletSigner.privateKey;

// Sign simple address message
const walletNewVg = await BlsWalletWrapper.connect(
blsSecret,
vg2.address,
vg2.provider,
);

const signatureExpiryTimestamp =
(await fx.provider.getBlock("latest")).timestamp +
safetyDelaySeconds +
signatureExpiryOffsetSeconds;
const addressMessage = solidityPack(
["address", "uint256"],
[walletAddress, signatureExpiryTimestamp],
);
const addressSignature = walletNewVg.signMessage(addressMessage);

const proxyAdmin2Address = await vg2.walletProxyAdmin();
// Get admin action to change proxy
const bundle = await proxyAdminBundle(fx, walletOldVg, "changeProxyAdmin", [
walletAddress,
proxyAdmin2Address,
]);
const changeProxyAction = bundle.operations[0].actions[0];

// prepare call
const txnReceipt = await proxyAdminCall(
fx,
walletOldVg,
"changeProxyAdmin",
[walletAddress, proxyAdmin2Address],
);
expectOperationsToSucceed(txnReceipt);

// Advance time one week
await fx.advanceTimeBy(safetyDelaySeconds + 1);

const hash = walletOldVg.blsWalletSigner.getPublicKeyHash();

const setExternalWalletAction: ActionData = {
ethValue: BigNumber.from(0),
contractAddress: vg2.address,
encodedFunction: vg2.interface.encodeFunctionData("setBLSKeyForWallet", [
addressSignature,
walletOldVg.PublicKey(),
signatureExpiryTimestamp,
]),
};

const setTrustedBLSGatewayAction: ActionData = {
ethValue: BigNumber.from(0),
contractAddress: fx.verificationGateway.address,
encodedFunction: fx.verificationGateway.interface.encodeFunctionData(
"setTrustedBLSGateway",
[hash, vg2.address],
),
};

// Upgrading the gateway requires these three steps:
// 1. register external wallet in vg2
// 2. change proxy admin to that in vg2
// 3. lastly, set wallet's new trusted gateway
//
// If (1) or (2) are skipped, then (3) should fail, and therefore the whole
// operation should fail.

{
// Fail if setExternalWalletAction is skipped

const { successes } =
await fx.verificationGateway.callStatic.processBundle(
walletOldVg.sign({
nonce: BigNumber.from(1),
gas: BigNumber.from(30_000_000),
actions: [
// skip: setExternalWalletAction,
changeProxyAction,
setTrustedBLSGatewayAction,
],
}),
);

expect(successes).to.deep.equal([false]);
}

{
// Fail if changeProxyAction is skipped

const { successes } =
await fx.verificationGateway.callStatic.processBundle(
walletOldVg.sign({
nonce: BigNumber.from(1),
gas: BigNumber.from(30_000_000),
actions: [
setExternalWalletAction,
// skip: changeProxyAction,
setTrustedBLSGatewayAction,
],
}),
);

expect(successes).to.deep.equal([false]);
}

{
// Succeed if nothing is skipped

const { successes } =
await fx.verificationGateway.callStatic.processBundle(
walletOldVg.sign({
nonce: BigNumber.from(1),
gas: BigNumber.from(30_000_000),
actions: [
setExternalWalletAction,
changeProxyAction,
setTrustedBLSGatewayAction,
],
}),
);

expect(successes).to.deep.equal([true]);
}

await expect(vg2.walletFromHash(hash)).to.eventually.not.equal(
walletAddress,
);

// Now actually perform the upgrade so we can perform some more detailed
// checks.
await fx.processBundleWithExtraGas(
walletOldVg.sign({
nonce: BigNumber.from(1),
gas: BigNumber.from(30_000_000),
actions: [
setExternalWalletAction,
changeProxyAction,
setTrustedBLSGatewayAction,
],
}),
);

// Create required objects for data/contracts for checks
const proxyAdmin = await ethers.getContractAt(
"ProxyAdmin",
await vg2.walletProxyAdmin(),
);

// Direct checks corresponding to each action
await expect(vg2.walletFromHash(hash)).to.eventually.equal(walletAddress);
await expect(vg2.hashFromWallet(walletAddress)).to.eventually.equal(hash);
await expect(proxyAdmin.getProxyAdmin(walletAddress)).to.eventually.equal(
proxyAdmin.address,
);
expectPubkeysEql(
await getPublicKeyFromHash(vg2, hash),
walletOldVg.PublicKey(),
);

const blsWallet = await ethers.getContractAt("BLSWallet", walletAddress);
// New verification gateway pending
await expect(blsWallet.trustedBLSGateway()).to.eventually.equal(
fx.verificationGateway.address,
);
// Advance time one week
await fx.advanceTimeBy(safetyDelaySeconds + 1);
// set pending
await (await blsWallet.setAnyPending()).wait();
// Check new verification gateway was set
await expect(blsWallet.trustedBLSGateway()).to.eventually.equal(
vg2.address,
);

await walletNewVg.syncWallet(vg2);
// Check new gateway has wallet via static call through new gateway
const bundleResult = await vg2.callStatic.processBundle(
fx.blsWalletSigner.aggregate([
walletNewVg.sign({
nonce: BigNumber.from(2),
gas: BigNumber.from(30_000_000),
actions: [
{
ethValue: 0,
contractAddress: vg2.address,
encodedFunction: vg2.interface.encodeFunctionData(
"walletFromHash",
[hash],
),
},
],
}),
]),
);
const walletFromHashAddress = ethers.utils.defaultAbiCoder.decode(
["address"],
bundleResult.results[0][0], // first and only operation/action result
)[0];
expect(walletFromHashAddress).to.equal(walletAddress);
// Still possible to point wallets to a new gateway if desired, just not with v1 deployment
});

it("should change mapping of an address to hash", async () => {
Expand Down
Loading