Add Multi-sig Vault SC #237

icyblob · 2024-12-11T05:17:06Z

Purpose

The MSVAULT (Multi-Signature Vault) smart contract is designed as a secure vault for Qubic that requires multiple parties to manage and approve fund releases. By enabling multiple owners per vault and enforcing specific consensus rules (such as 2/3 quorum or two-out-of-x approvals), MSVAULT will make sure that no single party can unilaterally withdraw funds. This empowers groups to hold and manage assets collectively with strong on-chain security guarantees.

Core Design Features

Multiple Ownership and management:
MSVAULT supports up to 32 owners per vault. Each owner can submit requests to release funds, and the contract enforces that only after the required number of owner approvals are given will the funds actually move. This prevents unauthorized access, ensuring a safer environment for pooled capital.
Two Vault Types for Flexibility:
MSVAULT offers two distinct types of multi-sig management:
- Type 1: Quorum rule (2/3 threshold). For N owners, releasing funds requires at least two-thirds to request the exact same release (same vault, same amount, same destination).
- Type 2: Two-out-of-X owners rule. Any two distinct owners out of the total set (if they match on amount and destination) can trigger a release.
Auditable and Transparent:
All actions—registration, deposits, and release requests—are recorded on-chain. Users can query states via public functions to see vault balances, vault ownership, and pending release requests. This transparency fosters trust and easy verification of vault activities.
Fees and Incentives:
The contract charges fees to create new vaults, release funds, and reset release requests. These fees discourage spam and generate revenue for the network, some of which may be distributed to shareholders or burned, contributing to network sustainability.
Automatic Maintenance:
At the end of each epoch, the contract charges a small holding fee. If a vault cannot afford this fee, it gets destroyed and removed from active storage. This ensures that inactive or underfunded vaults do not consume resources indefinitely. Freed slot can be reused for a future vault.

Core Functionalities

Procedures (Invoked by Transactions)

register(vaultType, vaultName, owners...):
- Action: Creates a new vault with the specified owners and vault name.
- Conditions:
  - Requires a REGISTERING_FEE paid by the caller.
  - If only one owner is specified, the call returns the funds and does nothing (no single-owner vaults).
  - Vault type must be either 1 (2/3 quorum) or 2 (two-out-of-x).
- Output: Returns uint64 vaultID identifying the newly registered vault.
- Storage: Stores vault metadata (owners, name, type) and initializes it as active.
deposit(vaultID):
- Action: Deposit QUs directly into the specified vault. The vault’s balance increases by the amount sent in this transaction.
releaseTo(vaultID, amount, destination):
- Action: Owner requests a release of amount QUs to a given destination.
- Rules:
  - Caller must be an owner of vaultID.
  - If multiple owners request the exact same release parameters (vaultID, amount, destination), once the rule threshold is reached:
    - The contract transfers the specified amount to destination.
    - All stored requests for that release are reset.
  - Charges a RELEASE_FEE to prevent spam.
  - If amount is zero or destination is NULL_ID, the request is ignored.
resetRelease(vaultID):
- Action: Resets the release request state for the calling owner within the given vault.
- Purpose: Allows an owner to revoke their request without waiting for epoch end.
- Fee: A small RELEASE_RESET_FEE is charged.

Automatic Epoch-End Maintenance

END_EPOCH: For each active vault, a HOLDING_FEE is taken. If a vault’s balance is insufficient:
- The vault is destroyed.
- Freed slot can be reused for a future vault.

View Functions (No funds Required)

getVaults(publicKey):
- Output: Returns a list of vaultIDs and corresponding vaultNames that publicKey owns.
- Use Case: Allows any user to quickly see which vaults they participate in.
getReleaseStatus(vaultID):
- Output: Shows each owner’s pending release request (amount and destination).
- Use Case: Owners and auditors can see how close a release is to being approved.
getBalanceOf(vaultID):
- Output: The current QUs balance of the specified vault.
- Use Case: Quick check of vault funds.
getVaultName(vaultID):
- Output: The stored name of the vault.
- Use Case: User-friendly identification of a vault.
getRevenueInfo():
- Output: Basic stats like:
  - Number of active vaults,
  - Total revenue earned by the contract,
  - Total distributions to shareholders, etc.
- Use Case: Network participants can gauge contract activity and profitability.

Security Considerations

Multi-Owner Approval:
The core security model relies on multiple owners approving the same release request. This reduces the risk of a single compromised owner stealing funds.
Fees to Prevent Spam:
Introducing RELEASE_FEE and RELEASE_RESET_FEE ensures owners don’t spam release requests, maintaining stable contract performance and reducing denial-of-service risks.
Epoch-Based Maintenance:
Regular END_EPOCH checks ensure dormant or depleted vaults cannot remain indefinitely. This prevents “storage bloating” and improves state sanity.
Proper Input Validation:
The contract ignores invalid requests (e.g., zero amount or NULL destination) and ensures callers must be owners. This reduces exploitable edge cases.

How the Contract Works Step-by-Step

Vault Creation:
A user calls register(...) with multiple owners and a REGISTERING_FEE. The contract assigns a vaultID and records the vault’s info. The vault now securely holds QUs.
Deposits:
Owners or any external party can deposit funds by calling deposit(vaultID) and including QUs. The vault balance increases on-chain, controlled by the MSVAULT contract’s logic.
Releasing Funds:
Suppose 5 owners (A, B, C, D, E) manage the vault. A tries releaseTo(...) with X QUs to dest1. If the vault is type 1 (2/3 quorum), at least 4 owners must request the exact same X and dest1. Upon reaching that threshold, funds are released immediately. For a type 2 vault, only 2 owners out of total owners are needed.
Resetting Requests:
If an owner changes their mind, they call resetRelease(vaultID), removing their part of a pending request. This action costs a small fee, ensuring it’s not abused.
Epoch-End Maintenance:
Periodically, the contract deducts a HOLDING_FEE from each vault. If a vault cannot pay, it is closed and freed, ensuring the contract’s state remains manageable.

CLI interface

qubic/qubic-cli#50

Frontend Integration Requirements

Dashboard:
- A dashboard that lists all vaults a user is involved in (via getVaults(publicKey)).
- Clear visualization of pending release requests and the number of approvals needed.
Transaction Builder:
- Simple forms for register(...) that allow selecting vault type, specifying vault name, and adding multiple owners.
- A deposit form to easily add funds to a vault.
- A release request form to specify amount and destination.
- A reset button to cancel pending requests.
Real-Time Updates:
- Periodically call getReleaseStatus, getBalanceOf, and getVaultName to keep UI updated.
Error Handling and Confirmation:
- Handle errors where a user tries to release funds but isn’t an owner.
- Confirmations when thresholds are met and funds are released.
Security and Education:
- Frontend should explain the consensus rules to users (2/3 quorum or 2-out-of-x).
- Encourage owners to safely store their keys off-line, as their approval power is critical.

Constants

MAX_OWNERS = 32:
Limits the maximum number of owners per vault, simplifying the state and ensuring efficient handling of approvals.
INITIAL_MAX_VAULTS = 1024 and MAX_VAULTS = INITIAL_MAX_VAULTS * X_MULTIPLIER:
Sets initial and scalable capacity for vault creation, allowing the contract to handle a growing number of vaults while maintaining performance.
REGISTERING_FEE = 10000ULL:
Required to create a vault, discouraging spam and ensuring only serious users allocate resources to occupy on-chain space.
RELEASE_FEE = 1000ULL:
Imposed when initiating a fund release request, preventing spam by making repetitive or frivolous attempts costly.
RELEASE_RESET_FEE = 500ULL:
Charged when an owner resets their release request, balancing flexibility with responsibility and preventing constant toggling of requests.
HOLDING_FEE = 100ULL:
Collected at the end of each epoch from each active vault. Insufficient funds result in vault deactivation, ensuring no stale vaults linger indefinitely.

Project link: https://github.com/orgs/qubic/projects/1/views/1?filterQuery=subproject%3ACore&pane=issue&itemId=88813217

icyblob · 2024-12-14T06:00:01Z

@philippwerner This PR is ready to be reviewed. Thank you.

philippwerner

Thanks for your contribution. I had a quick lock through some of your code and added a view comments. Please apply these to all similar cases in the contract (all temporary vars should be in the locals struct, none on the function call stack.

In case you don't know it yet, please read: https://github.com/qubic/core/blob/main/doc/contracts.md

I will continue reviewing later.

src/contracts/MsVault.h

J0ET0M · 2024-12-16T10:23:57Z

src/contracts/MsVault.h

+
+    struct MSVaultLogger
+    {
+        unsigned int _contractIndex;


would it make sense to not mix between built in types an qpi types? (i mean use consequently qpi tpyes)

J0ET0M · 2024-12-16T19:33:51Z

src/contracts/MsVault.h

+
+    struct getReleaseStatus_input
+    {
+        uint64 vaultID;


the term vaultID is used at several places.
internally it is the the index, one could think about name it also index to make it also clear for anyone which is using the SC.

if we keep id, i personally would also write vaultId instead of vaultID to keep it all in camal case

I have renamed to vaultId to keep it as camel case.

J0ET0M

nice contract, thanks!

J0ET0M · 2024-12-16T19:42:03Z

src/contracts/MsVault.h

+            LOG_INFO(loggerMsg);
+            return;
+        }
+


i would not let users to create a release request when the current balance is to low. therefore i would check this before and reject the request.

Now the function will check for balance and reject before setting the release request. It will still check the balance again before transfering the funds.

J0ET0M · 2024-12-16T19:54:38Z

src/contracts/MsVault.h

+        loggerMsg.amount = 0;
+        loggerMsg.destination = NULL_ID;
+
+        if (input.vaultID >= MAX_VAULTS)


the following check sections are repeated at least twice. maybe a helper method could unify them.

I have created a helper function isValidVaultId and replace about 6 places that check for vaultId validity

J0ET0M · 2024-12-16T20:00:07Z

src/contracts/MsVault.h

+    PUBLIC_FUNCTION_WITH_LOCALS(getReleaseStatus)
+        if (input.vaultID >= MAX_VAULTS)
+        {
+            for (locals.i = 0; locals.i < MAX_OWNERS; locals.i++)


instead of setting all just to 0 you could also add a status to the output that let the user identify that the requested vault is invalid otherwise it could be a user error because he requested wrong vault index

the same can apply to the other conditions where the user will not receive a valid result. e.g. inactive vault

same could also apply to getBalanceOf, getVaultName

Done for all these functions getReleaseStatus, getBalanceOf, and getVaultName

J0ET0M · 2024-12-16T20:04:25Z

src/contracts/MsVault.h

+    BEGIN_EPOCH_WITH_LOCALS
+        for (locals.i = 0; locals.i < MAX_VAULTS; locals.i++)
+        {
+            Vault v = state.vaults.get(locals.i);


i guess also here: assign this to locals.vault @philippwerner ?

J0ET0M · 2024-12-16T20:05:42Z

src/contracts/MsVault.h

+            if (v.isActive)
+            {
+                locals.rr_in.vault = v;
+                resetReleaseRequests(qpi, state, locals.rr_in, locals.rr_out, locals.rr_locals);


why should the release request not being kept over epochs?

It is specified in the project description here

Please correct me if I understand it wrong.

@krypdkat what do you think?
after rethinking it makes more sense to keen the requests. from users perspective it is best practice that they can send release requests at any time. may be adding a timeout, but for simplicity i would just keep it active until manual reset or acceptance.

@J0ET0M the main purpose was only trying to make a clean state at every epoch begin (easier debugging and calculation), nothing more.
So if the code and other logic are concrete, then reseting storage is not needed.

understand. then let's make the sure the code is clean and we can have release requests stay over epochs. from a user perspective this is def. something which is needed. @icyblob

cyber-pc · 2024-12-17T03:01:26Z

src/contracts/MsVault.h

+constexpr uint64 REGISTERING_FEE = 10000ULL;
+constexpr uint64 RELEASE_FEE = 1000ULL;
+constexpr uint64 RELEASE_RESET_FEE = 500ULL;
+constexpr uint64 HOLDING_FEE = 100ULL;


[Minor] Should we allow getting those fees from client side (cli, front-end) ?

I have added a function to get all the fees.
core side: 3e4a994
cli side: qubic/qubic-cli@a1424b2

icyblob · 2024-12-17T23:24:30Z

@philippwerner @J0ET0M @cyber-pc Thank you for your reviewing. I have addressed all the comments.

krypdkat

have a few comments, overall lgtm

krypdkat · 2024-12-23T19:37:30Z

src/contracts/MsVault.h

+using namespace QPI;
+
+constexpr uint16 MSVAULT_MAX_OWNERS = 32;
+constexpr uint16 MSVAULT_INITIAL_MAX_VAULTS = 1024;


is this too small, can we pump this number up?

I have pump this number to 2048. Please let me know if you think this is still small.

what's the state size at 2048? Just need to make it close to 1GiB

krypdkat · 2024-12-23T19:38:51Z

src/contracts/MsVault.h

+    PUBLIC_FUNCTION_WITH_LOCALS(getVaults)
+        output.numberOfVaults = 0;
+        locals.count = 0;
+        for (locals.i = 0; locals.i < 1024; locals.i++)


1024 => MSVAULT_INITIAL_MAX_VAULTS
and other places as well

krypdkat · 2024-12-23T19:41:29Z

src/contracts/MsVault.h

+                else
+                {
+                    // Not enough funds to pay holding fee
+                    locals.v.isActive = false;


the SC takes everything left. eg: fee is 10 coins, vault has 7 coins => SC takes 7 coins and then destroy the vault. By destroy I mean: reset everything about that vault to zero and empty.

- Bump up max vaults to 2048 - Change max vault data type to uint64 and other related variables' types - Adjsut max owners to 16 - Add max co-owners to 8 and reject any vault registration if having any owners exceed that limit - If the vault can't afford holding fee, take all that left, and reset EVERYTHING to 0 - Adjust the dividend payment logic - Change 1 input type for registerVault to fix misalignment issue

icyblob force-pushed the msvault branch 3 times, most recently from 2e8198f to 8fa006a Compare December 13, 2024 06:33

icyblob mentioned this pull request Dec 14, 2024

Add cli for Multi-sig vault SC qubic/qubic-cli#50

Open

philippwerner reviewed Dec 15, 2024

View reviewed changes

J0ET0M reviewed Dec 16, 2024

View reviewed changes

cyber-pc reviewed Dec 17, 2024

View reviewed changes

icyblob force-pushed the msvault branch 2 times, most recently from bcd0ca6 to b8e2f6a Compare December 19, 2024 16:13

krypdkat requested changes Dec 23, 2024

View reviewed changes

icyblob force-pushed the msvault branch 3 times, most recently from a398be5 to 7ef45e1 Compare December 28, 2024 03:06

icyblob force-pushed the msvault branch from 7ef45e1 to 5f684c1 Compare January 10, 2025 23:11

icyblob force-pushed the msvault branch 2 times, most recently from d087b1c to 3949967 Compare January 29, 2025 05:25

icyblob added 10 commits January 29, 2025 05:25

Add Multi-sig Vault SC

ac73046

Remove all local variables inside functions

b012879

Rename constants, remove redundant, add access specifiers

0457867

Rename vaultID to vaultId

86602c6

Check balance before proceed releaseRequest

1a3196a

Use helper function to verify vaultId

d6d94a7

Add status field for viewing functions

9551bc4

Add getFees function

586f944

Add GetRevenue Test for msvault unit test

591c78f

icyblob added 11 commits January 29, 2025 05:25

Add log init for MsVault unit test

a2baf46

Misc changes

9095e58

MAX VAULTS to 2^19

72cab3b

Add get vault owners

cdf91d8

Keep the release status to next epoch

4789a3e

Rename array to Array

f8ba86c

Remove vault type, add y of X for approval

d345e21

Output requiredApproval in getVaultOwners

9b27540

Fix unit test

a17bd41

status from bit to uint64

3949967

requiredApprovals must be > 1

9dc990d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Multi-sig Vault SC #237

Add Multi-sig Vault SC #237

icyblob commented Dec 11, 2024 •

edited

Loading

icyblob commented Dec 14, 2024

philippwerner left a comment

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M left a comment

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M Dec 16, 2024

J0ET0M Dec 16, 2024

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M Dec 16, 2024

icyblob Dec 17, 2024

J0ET0M Dec 26, 2024

krypdkat Dec 27, 2024

J0ET0M Jan 4, 2025 •

edited

Loading

cyber-pc Dec 17, 2024 •

edited

Loading

icyblob Dec 17, 2024

icyblob commented Dec 17, 2024

krypdkat left a comment

krypdkat Dec 23, 2024

icyblob Dec 27, 2024

krypdkat Jan 6, 2025

krypdkat Dec 23, 2024

icyblob Dec 27, 2024

krypdkat Dec 23, 2024

icyblob Dec 27, 2024

Add Multi-sig Vault SC #237

Are you sure you want to change the base?

Add Multi-sig Vault SC #237

Conversation

icyblob commented Dec 11, 2024 • edited Loading

Purpose

Core Design Features

Core Functionalities

Procedures (Invoked by Transactions)

Automatic Epoch-End Maintenance

View Functions (No funds Required)

Security Considerations

How the Contract Works Step-by-Step

CLI interface

Frontend Integration Requirements

Constants

icyblob commented Dec 14, 2024

philippwerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J0ET0M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J0ET0M Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

cyber-pc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icyblob commented Dec 17, 2024

krypdkat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icyblob commented Dec 11, 2024 •

edited

Loading

J0ET0M Jan 4, 2025 •

edited

Loading

cyber-pc Dec 17, 2024 •

edited

Loading