Omniscia vfat Audit

SickleMultisig Manual Review Findings

SickleMultisig Manual Review Findings

SMG-01M: Inexistent Transaction Expiry / Sequence

Description:

A transaction submitted in the SickleMultisig does not expire as long as the members of the multi-signature wallet remain the same.

Additionally, the sequence in which transactions are executed is not enforced by the system, permitting them to be executed in an arbitrary order.

Impact:

While multi-signature wallet signers will collectively sign which transactions are meant to be executed, control over their order might allow one of the signers to benefit from the wallet at the expense of others.

Example:

contracts/governance/SickleMultisig.sol
18struct Transaction {
19 // Calls to be executed in the transaction
20 Proposal proposal;
21 // Transaction state
22 bool exists;
23 bool executed;
24 bool cancelled;
25 // Settings nonce that the transaction was created with
26 uint256 settingsNonce;
27 // Signing state
28 uint256 signatures;
29 mapping(address => bool) signed;
30}

Recommendation:

We advise both issues to be rectified via a configurable time-based expiry for transactions and a wallet-wide nonce system that would supercede the settingsNonce variable's functionality.

Alleviation (6ab7af3bb4):

The vfat team proceeded with introducing a transaction expiry system via a contract-wide configurable expiration date offset, ensuring transactions cannot perpetually linger in the contract.

To note, the vfat team has not introduced a transaction ordering system and did not provide adequate justification as to why this is undesirable, rendering the original exhibit partially addressed.

Alleviation (986c6b0a71):

The vfat team evaluated our follow-up recommendation and clarified that they do not intend to apply a sequential execution system and that they have deliberately set up the system to execute transactions in any order.

As such, we consider the previous commit's alleviation to have been sufficient, rendering this exhibit fully addressed.