Skip to content

Commit

Permalink
Merge pull request #5 from cloudwalk/audit-updates
Browse files Browse the repository at this point in the history 
Changes madden after the first iteration of the audit.
  • Loading branch information
@igorsenych-cw
igorsenych-cw authored Mar 24, 2023
2 parents b5d6c2b + 01c8b91 commit 91605cf
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 147 deletions.
22 changes: 21 additions & 1 deletion contracts/MultiSigWalletUpgradeable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@
pragma solidity 0.8.16;

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

import { MultiSigWalletBase } from "./base/MultiSigWalletBase.sol";

/**
* @title MultiSigWalletUpgradeable contract
* @author CloudWalk Inc.
* @dev The implementation of the upgradeable multi-signature wallet contract.
*/
contract MultiSigWalletUpgradeable is Initializable, MultiSigWalletBase {
contract MultiSigWalletUpgradeable is Initializable, UUPSUpgradeable, MultiSigWalletBase {
/**
* @dev Constructor that prohibits the initialization of the implementation of the upgradable contract.
*
Expand Down Expand Up @@ -58,7 +60,25 @@ contract MultiSigWalletUpgradeable is Initializable, MultiSigWalletBase {
internal
onlyInitializing
{
__UUPSUpgradeable_init_unchained();
_configureExpirationTime(365 days);
_configureOwners(newOwners, newRequiredApprovals);
}

/**
* @dev Upgrade authorization function.
*
* See details: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable
*
* @param newImplementation The address of the new implementation
*
* Requirements:
*
* - The caller must be the multisig itself.
*/
function _authorizeUpgrade(address newImplementation)
internal
onlySelfCall
override
{}
}
11 changes: 11 additions & 0 deletions contracts/base/MultiSigWalletBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import { MultiSigWalletStorage } from "./MultiSigWalletStorage.sol";
* @dev The base of the multi-signature wallet contract.
*/
abstract contract MultiSigWalletBase is MultiSigWalletStorage, IMultiSigWallet {
// --------------------------- Constants ---------------------------

/// @dev The minimal transaction expiration time.
uint256 public constant MINIMUM_EXPIRATION_TIME = 60 minutes;

// --------------------------- Errors ---------------------------

/// @dev An unauthorized account called a function.
Expand Down Expand Up @@ -52,6 +57,9 @@ abstract contract MultiSigWalletBase is MultiSigWalletStorage, IMultiSigWallet {
/// @dev A transaction with the specified Id is on cooldown.
error CooldownNotEnded();

/// @dev The invalid amount of time was passed when configuring the expiration time.
error InvalidExpirationTime();

// ------------------------- Modifiers --------------------------

/**
Expand Down Expand Up @@ -454,6 +462,9 @@ abstract contract MultiSigWalletBase is MultiSigWalletStorage, IMultiSigWallet {
* @dev See {MultiSigWallet-configureExpirationTime}.
*/
function _configureExpirationTime(uint120 newExpirationTime) internal {
if (newExpirationTime < MINIMUM_EXPIRATION_TIME) {
revert InvalidExpirationTime();
}
_expirationTime = newExpirationTime;
emit ConfigureExpirationTime(newExpirationTime);
}
Expand Down
115 changes: 58 additions & 57 deletions docs/documentation.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,105 +26,106 @@ BRLC-multisig is a multi-signature wallet used to ensure the decentralization an

Function `submit` - submits new transaction and adds it to transactions array. Emits a `Submit` event. Can be called only by the owner.

Function `submitAndApprove` - submits new transaction and adds it to transactions array. Emits a `Submit` event. Approves submitted transaction. Emits an `Approve` event. Should be called only by the owner.
Function `submitAndApprove` - submits new transaction and adds it to transactions array. Emits a `Submit` event. Approves submitted transaction. Emits an`Approve` event. Can be called only by the owner.
<ul>
<li>Should be reverted if the selected transaction does not exist.</li>
<li>Should be reverted if the selected transaction is expired.</li>
<li>Should be reverted if the selected transaction is executed.</li>
<li>Should be reverted if the selected transaction is already approved by the caller.</li>
<li>Reverts if the selected transaction does not exist.</li>
<li>Reverts if the selected transaction is expired.</li>
<li>Reverts if the selected transaction is executed.</li>
<li>Reverts if the selected transaction is already approved by the caller.</li>
</ul>

Function `approve` - approves selected transaction. Emits an `Approve` event. Should be called only by the owner.
Function `approve` - approves selected transaction. Emits an `Approve` event. Can be called only by the owner.
<ul>
<li>Should be reverted if the selected transaction does not exist.</li>
<li>Should be reverted if the selected transaction is expired.</li>
<li>Should be reverted if the selected transaction is executed.</li>
<li>Should be reverted if the selected transaction is already approved by the caller.</li>
<li>Reverts if the selected transaction does not exist.</li>
<li>Reverts if the selected transaction is expired.</li>
<li>Reverts if the selected transaction is executed.</li>
<li>Reverts if the selected transaction is already approved by the caller.</li>
</ul>

Function `approveAndExecute` - approves and executes the selected transaction. Emits an `Approve` event. Executes transaction. Emits an `Execute` event. Should be called only by the owner.
Function `approveAndExecute` - approves and executes the selected transaction. Emits an `Approve` event. Executes transaction. Emits an `Execute` event. Can be called only by the owner.
<ul>
<li>Should be reverted if the selected transaction does not exist.</li>
<li>Should be reverted if the selected transaction is expired.</li>
<li>Should be reverted if the selected transaction is executed.</li>
<li>Should be reverted if the selected transaction is already approved by the caller.</li>
<li>Should be reverted if the selected transaction is executed.</li>
<li>Should be reverted if the selected transaction is on cooldown.</li>
<li>Should be reverted if the approvals amount is less than the amount of required approvals.</li>
<li>Reverts if the selected transaction does not exist.</li>
<li>Reverts if the selected transaction is expired.</li>
<li>Reverts if the selected transaction is executed.</li>
<li>Reverts if the selected transaction is already approved by the caller.</li>
<li>Reverts if the selected transaction is executed.</li>
<li>Reverts if the selected transaction is on cooldown.</li>
<li>Reverts if the approvals amount is less than the amount of required approvals.</li>
</ul>

Function `execute` - executes the selected transaction. Emits an `Execute` event. Should be called only by the owner.
Function `execute` - executes the selected transaction. Emits an `Execute` event. Can be called only by the owner. Allows repeating execution attempt if previous execution failed. Owners are able to choose the order of the execution of approved transactions.
<ul>
<li>Should be reverted if the selected transaction does not exist.</li>
<li>Should be reverted if the selected transaction is expired.</li>
<li>Should be reverted if the selected transaction is executed.</li>
<li>Should be reverted if the selected transaction is on cooldown.</li>
<li>Should be reverted if the approvals amount is less than the amount of required approvals.</li>
<li>Should be reverted if the transaction execution fails.</li>
<li>Reverts if the selected transaction does not exist.</li>
<li>Reverts if the selected transaction is expired.</li>
<li>Reverts if the selected transaction is executed.</li>
<li>Reverts if the selected transaction is on cooldown.</li>
<li>Reverts if the approvals amount is less than the amount of required approvals.</li>
<li>Reverts if the transaction execution fails.</li>
</ul>

Function `revoke` - revokes approval from the selected transaction. Emits a `Revoke` event. Should be called only by the owner.
Function `revoke` - revokes approval from the selected transaction. Emits a `Revoke` event. Can be called only by the owner.
<ul>
<li>Should be reverted if the selected transaction does not exist.</li>
<li>Should be reverted if the selected transaction is expired.</li>
<li>Should be reverted if the selected transaction is executed.</li>
<li>Should be reverted if the selected transaction is not approved by the caller.</li>
<li>Reverts if the selected transaction does not exist.</li>
<li>Reverts if the selected transaction is expired.</li>
<li>Reverts if the selected transaction is executed.</li>
<li>Reverts if the selected transaction is not approved by the caller.</li>
</ul>

Function `configureOwners` - changes owners array and amount of required approvals. Emits a `ConfigureOwners` event.
Function `configureOwners` - changes owners array and amount of required approvals. Emits a `ConfigureOwners` event. Function execution does not change the state of submitted transactions, the amount of approvals made by previous owners will stay the same.
<ul>
<li>Should be reverted if the caller is not a multisig itself.</li>
<li>Should be reverted if the array of owners is empty.</li>
<li>Should be reverted if one of the owners is zero address.</li>
<li>Should be reverted if the owner address is duplicated.</li>
<li>Should be reverted if the number of required approvals is bigger than the amount of owners.</li>
<li>Should be reverted if the number of required approvals is zero.</li>
<li>Reverts if the caller is not a multisig itself.</li>
<li>Reverts if the array of owners is empty.</li>
<li>Reverts if one of the owners is zero address.</li>
<li>Reverts if the owner address is duplicated.</li>
<li>Reverts if the number of required approvals is bigger than the amount of owners.</li>
<li>Reverts if the number of required approvals is zero.</li>
</ul>

Function `configureExpirationTime` - changes default expiration time of transactions. Emits a `ConfigureExpirationTime` event.
Function `configureExpirationTime` - changes default expiration time of transactions. Emits a `ConfigureExpirationTime` event. Can be any amount of time bigger than the allowed minimum.
<ul>
<li>Should be reverted if the caller is not a multisig itself.</li>
<li>Reverts if the caller is not a multisig itself.</li>
<li>Reverts if the passed expiration time is less than the minimum allowed</li>
</ul>


Function `configureCooldownTime` - changes default cooldown time of transactions. Emits a `ConfigureCooldownTime` event.
<ul>
<li>Should be reverted if the caller is not a multisig itself.</li>
<li>Reverts if the caller is not a multisig itself.</li>
</ul>


### [```MultiSigWallet.sol```](../contracts//MultiSigWallet.sol)

`constructor` - sets the owners of the multisig, number of required approvals and the expiration time (365 days by default).
<ul>
<li>Should be reverted if the array of owners is empty.</li>
<li>Should be reverted if one of the owners is zero address.</li>
<li>Should be reverted if the owner address is duplicated.</li>
<li>Should be reverted if the number of required approvals is bigger than the amount of owners.</li>
<li>Should be reverted if the number of required approvals is zero.</li>
<li>Reverts if the array of owners is empty.</li>
<li>Reverts if one of the owners is zero address.</li>
<li>Reverts if the owner address is duplicated.</li>
<li>Reverts if the number of required approvals is bigger than the amount of owners.</li>
<li>Reverts if the number of required approvals is zero.</li>
</ul>

### [```MultiSigWalletUpgradeable.sol```](../contracts//MultiSigWalletUpgradeable.sol)

Function `initialize` - initializes the contract with the selected parameters. Sets the owners of the multisig, number of required approvals and the expiration time (365 days by default).
<ul>
<li>Should be reverted if the array of owners is empty.</li>
<li>Should be reverted if one of the owners is zero address.</li>
<li>Should be reverted if the owner address is duplicated.</li>
<li>Should be reverted if the number of required approvals is bigger than the amount of owners.</li>
<li>Should be reverted if the number of required approvals is zero.</li>
<li>Should be reverted if the number of required approvals is zero.</li>
<li>Ownership of the ProxyAdmin contract will be transferred to the wallet itself, so the upgrade will be possible only after the transaction is approved by owners.</li>
<li>Upgrade can be called only by multisig itself.</li>
<li>Reverts if the array of owners is empty.</li>
<li>Reverts if one of the owners is zero address.</li>
<li>Reverts if the owner address is duplicated.</li>
<li>Reverts if the number of required approvals is bigger than the amount of owners.</li>
<li>Reverts if the number of required approvals is zero.</li>
<li>Reverts if the number of required approvals is zero.</li>
</ul>


### [```MultiSigWalletFactory.sol```](../contracts//MultiSigWalletFactory.sol)

Function `deployNewWallet` - creates new non-upgradeable instance of multisig wallet. Emits a `NewWallet` event.
<ul>
<li>Should be reverted if the array of owners is empty.</li>
<li>Should be reverted if one of the owners is zero address.</li>
<li>Should be reverted if the owner address is duplicated.</li>
<li>Should be reverted if the number of required approvals is zero.</li>
<li>Should be reverted if the number of required approvals is bigger than the amount of owners.</li>
<li>Reverts if the array of owners is empty.</li>
<li>Reverts if one of the owners is zero address.</li>
<li>Reverts if the owner address is duplicated.</li>
<li>Reverts if the number of required approvals is zero.</li>
<li>Reverts if the number of required approvals is bigger than the amount of owners.</li>
</ul>
45 changes: 29 additions & 16 deletions test/MultiSigWallet.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ async function setUpFixture(func: any) {

describe("MultiSigWallet contract", () => {
const REQUIRED_APPROVALS = 2;
const ONE_SECOND = 1;
const ONE_MINUTE = 60;
const TWO_HOURS = 7200;
const ONE_DAY = 3600 * 24;
const ONE_YEAR = 3600 * 24 * 365;

const ADDRESS_STUB = "0x0000000000000000000000000000000000000001";
Expand Down Expand Up @@ -458,16 +459,16 @@ describe("MultiSigWallet contract", () => {
describe("Function 'configureCooldownTime()'", () => {
it("Correctly changes the transaction cooldown time", async () => {
const { wallet } = await setUpFixture(deployWallet);
const txData = encodeConfigureCooldownTimeFunctionData(ONE_MINUTE);
const txData = encodeConfigureCooldownTimeFunctionData(TWO_HOURS);

await proveTx(
wallet.connect(owner1).submitAndApprove(wallet.address, 0, txData)
);
await expect(wallet.connect(owner2).approveAndExecute(0))
.to.emit(wallet, EVENT_NAME_CONFIGURE_COOLDOWN_TIME)
.withArgs(ONE_MINUTE);
.withArgs(TWO_HOURS);

expect(await wallet.cooldownTime()).to.eq(ONE_MINUTE);
expect(await wallet.cooldownTime()).to.eq(TWO_HOURS);
});

it("Is reverted if the caller is not the multi sig wallet itself", async () => {
Expand All @@ -485,16 +486,16 @@ describe("MultiSigWallet contract", () => {
it("Correctly changes the transaction expiration time", async () => {
const { wallet } = await setUpFixture(deployWallet);
const txData =
encodeConfigureExpirationTimeTimeFunctionData(ONE_MINUTE);
encodeConfigureExpirationTimeTimeFunctionData(ONE_YEAR);

await proveTx(
wallet.connect(owner1).submitAndApprove(wallet.address, 0, txData)
);
await expect(wallet.connect(owner2).approveAndExecute(0))
.to.emit(wallet, EVENT_NAME_CONFIGURE_EXPIRATION_TIME)
.withArgs(ONE_MINUTE);
.withArgs(ONE_YEAR);

expect(await wallet.expirationTime()).to.eq(ONE_MINUTE);
expect(await wallet.expirationTime()).to.eq(ONE_YEAR);
});

it("Is reverted if the caller is not the multi sig wallet itself", async () => {
Expand All @@ -506,6 +507,18 @@ describe("MultiSigWallet contract", () => {
REVERT_ERROR_IF_UNAUTHORIZED_CALLER
);
});

it("Is reverted if passed expiration time is less than minimal allowed", async () => {
const { wallet } = await setUpFixture(deployWallet);
const txData =
encodeConfigureExpirationTimeTimeFunctionData(ONE_MINUTE);

await proveTx(
wallet.connect(owner1).submitAndApprove(wallet.address, 0, txData)
);
await expect(wallet.connect(owner2).approveAndExecute(0))
.to.be.revertedWithCustomError(wallet, REVERT_ERROR_IF_INTERNAL_TRANSACTION_IS_FAILED)
});
});

describe("Function 'receive()'", () => {
Expand Down Expand Up @@ -1064,7 +1077,7 @@ describe("MultiSigWallet contract", () => {

it("Submission of a transaction sets the cooldown and expiration fields properly", async () => {
const { wallet } = await setUpFixture(deployWallet);
const txData = encodeConfigureCooldownTimeFunctionData(ONE_MINUTE);
const txData = encodeConfigureCooldownTimeFunctionData(TWO_HOURS);
const txId = await executeWalletTx({ wallet, txData });

const txReceipt = await proveTx(
Expand All @@ -1074,15 +1087,15 @@ describe("MultiSigWallet contract", () => {
const txStruct = await wallet.getTransaction(txId);
const block = await wallet.provider.getBlock(txReceipt.blockNumber);
const blockTimestamp: number = block.timestamp;
expect(txStruct.cooldown).to.eq(blockTimestamp + ONE_MINUTE);
expect(txStruct.cooldown).to.eq(blockTimestamp + TWO_HOURS);
expect(txStruct.expiration).to.eq(
blockTimestamp + ONE_MINUTE + ONE_YEAR
blockTimestamp + TWO_HOURS + ONE_YEAR
);
});

it("Execution of a transaction is reverted if the transaction is still on the cooldown", async () => {
const { wallet } = await setUpFixture(deployWallet);
const txData = encodeConfigureCooldownTimeFunctionData(ONE_MINUTE);
const txData = encodeConfigureCooldownTimeFunctionData(TWO_HOURS);
const txId = await executeWalletTx({ wallet, txData });

await proveTx(
Expand All @@ -1100,13 +1113,13 @@ describe("MultiSigWallet contract", () => {
it("Approval of a transaction is reverted if the transaction is already expired", async () => {
const { wallet } = await setUpFixture(deployWallet);
const txData =
encodeConfigureExpirationTimeTimeFunctionData(ONE_SECOND);
encodeConfigureExpirationTimeTimeFunctionData(ONE_DAY);
const txId = await executeWalletTx({ wallet, txData });

await proveTx(
wallet.connect(owner1).submitAndApprove(tx.to, tx.value, tx.data)
);
await wait(2 * ONE_SECOND);
await wait(2 * ONE_DAY);
await expect(
wallet.connect(owner2).approve(txId)
).to.be.revertedWithCustomError(
Expand All @@ -1130,7 +1143,7 @@ describe("MultiSigWallet contract", () => {
let txId = await executeWalletTx({ wallet, txData });

// Set the new expiration time
txData = encodeConfigureExpirationTimeTimeFunctionData(ONE_SECOND);
txData = encodeConfigureExpirationTimeTimeFunctionData(ONE_DAY);
await proveTx(
wallet.connect(owner1).submitAndApprove(wallet.address, 0, txData)
);
Expand All @@ -1145,7 +1158,7 @@ describe("MultiSigWallet contract", () => {
await proveTx(
wallet.connect(owner1).submitAndApprove(tx.to, tx.value, tx.data)
);
await wait(2 * ONE_SECOND);
await wait(2 * ONE_DAY);
await expect(
wallet.connect(owner1).execute(txId)
).to.be.revertedWithCustomError(
Expand All @@ -1160,7 +1173,7 @@ describe("MultiSigWallet contract", () => {
await proveTx(
wallet.connect(owner1).submitAndApprove(tx.to, tx.value, tx.data)
);
await wait(2 * ONE_SECOND);
await wait(2 * ONE_DAY);
await expect(
wallet.connect(owner1).revoke(txId)
).to.be.revertedWithCustomError(
Expand Down
Loading

0 comments on commit 91605cf

Please sign in to comment.