OneSig

There is no access control which prevents arbitrary callers from using the executeTransaction function. This is by design in the OneSig system, however this behavior introduces several risks.

Notably, when the multisig signers wish to invalidate a previous merkle proof with a seed change it is possible for an arbitrary user to frontrun the execution of this seed change or even the signature collection process for the merkle root of this seed change and execute the transactions which the signers wish to invalidate.

This can result in unexpected outcomes from the transactions which were desired to be skipped and furthermore the unexpected increment of the nonce for the OneSig contract.

Consider tracking a set of executor addresses which are allowed to invoke the executeTransaction function. The list of executors can be initialized to the set of signers and extended/decreased if the signers wish with an onlyMultiSig function.

USDT0 Team: Acknowledged.

The nonce for the OneSig contract is incremented directly before the execution of the set of external calls for the current transaction in the executeTransaction function. This successfully prevents replay of the current Transaction object through reentrancy in the executeTransaction function.

However if an arbitrary untrusted address were to gain control of the transaction execution during the execution of one of the external calls of the current Transaction, then that arbitrary untrusted address could re-enter and invoke the executeTransaction function again, now supplying the subsequent transaction in the same merkle root.

This would result in the transaction from the subsequent nonce, say nonce 2, being processed entirely in the middle of the external calls of the current nonce, say nonce 1.

This breaks the guarantee of atomicity for the external calls in a given Transaction object and ultimately could lead to unexpected behavior or even stolen funds/bricked systems.

Consider adding a nonReentrant modifier to the executeTransaction function.

USDT0 Team: Resolved.

The executeTransaction function may be invoked by any arbitrary address and does not include validation on the amount of gas that has been provided for each individual external call made as a part of the Transaction object.

Some external calls that are made from the OneSig contract may have different execution results depending on the amount of gas supplied.

For instance, if an invoked function includes a try/catch block around deeper logic, then the catch block can be trivially triggered by a malicious actor intentionally providing an insufficient amount of gas.

Additionally, there is no gas amount specified for each external call, therefore 63/64 of the transaction’s remaining gas is automatically forwarded to each external call.

This allows prior external calls to consume gas in such a way that can impact the execution of later external calls in a Transaction object.

Consider including an optional amount of gas that is to be present and forwarded to each external call in a Transaction in the leaf data so that if signers must ensure the amount of gas provided to an external call then this can be validated and provided as such.

USDT0 Team: Acknowledged.

The OneSigId serves to differentiate deployments of OneSig Multisigs across chains, thus preventing replay.

However in the event that a network fork occurs, transactions can be replayed across instances of a OneSig contract on both resulting chains because the OneSig immutable value remains the same in both instances.

Consider including the block.chainId as an enforced portion of the OneSigId and updating the OneSigId if the block.chainid is determined to have changed from an originally cached value. Similarly to the logic used in the _domainSeparatorV4 function of the OpenZeppelin EIP712 contract.

USDT0 Team: Acknowledged.

For L2 networks which use a sequencer it is possible for the block.timestamp used in a transaction to be out of date in cases where the sequencer is down for an extended period of time.

From the Arbitrum docs:

As mentioned, block timestamps are usually set based on the sequencer's clock.
Because there's a possibility that the sequencer fails to post batches on the
parent chain (i.e, Ethereum) for a period of time, it should have the ability
to slightly adjust the timestamp of the block to account for those delays and
prevent any potential reorganisations of the chain. To limit the degree to
which the sequencer can adjust timestamps, some boundaries are set, currently
to 24 hours earlier than the current time, and one hour in the future.

The behavior described here is that if the sequencer goes down for less than 24 hours, the sequencer can pick up any transactions from the Delayed Inbox and block.timestamp will be equal to the current time.

However, if the sequencer is down for more than 24 hours in extreme scenarios, then any forceInclusion transaction will be given a timestamp of the most recent L2 block, which is likely delayed significantly in this case, or the timestamp of the L1 from when the tx was placed in Delayed Inbox, whichever is greater. Consequently, a stale transaction could be executed past the expiry.

Be aware of this risk during an extended sequencer outage, and consider implementing a sequencer uptime check for L2 deployments which use a sequencer.

USDT0 Team: Acknowledged.

In the event that multiple valid and signed merkle roots exist for the same OneSig safe that have overlapping nonces in their transactions, an arbitrary user can invoke the executeTransaction function mixing and matching transactions from each merkle root to create an unexpected execution outcome.

Ensure that users are made aware of this risk and thus are careful to not sign merkle roots with overlapping nonces for the same OneSig contract.

USDT0 Team: Acknowledged.

The OneSig contract may be used to store native tokens on behalf of the signers for the contract, however because the executeTransaction function can be called by anyone this amount may be unexpectedly decreased in some scenarios.

If a merkle root which has been signed by the signers contains a function call with nonzero value, the signers may expect that the execution of executeTransaction would include that value as the msg.value of the transaction and thus not decrease the balance of the OneSig.

However an arbitrary user may invoke executeTransaction without providing any msg.value and thus decrease the OneSig native token balance by the transactions value. This may be unexpected for the signers and can deviate from exactly how they would like this transaction to be executed.

If this behavior is acceptable, then be sure to document it clearly for users of OneSig. Otherwise consider allowing the signers to specify as a part of the leaf data whether the native tokens should be supplied by the caller or the OneSig contract.

Alternatively, consider tracking a set of executor addresses which are allowed to invoke the executeTransaction function.

USDT0 Team: Acknowledged.

In the MultiSig contract the value of 65 is referred to multiple times to indicate the length of the signatures provided. However rather than using a “magic number” to represent this length, a constant value can be declared in the contract.

Consider declaring a SIGNATURE_LENGTH constant to replace the bespoke instances of 65.

USDT0 Team: Resolved.

In the verifyNSignatures function the amount of signatures is validated to be exactly the _threshold provided. However in the documentation it is described that:

Modifying signers must require the same signerThreshold (or more) of signatures as executing a transaction.

The optionality for a OneSig contract to require more than the threshold to initiate a signer change does not currently exist in the EVM version and is specifically disallowed by the validation performed on the _threshold and _signatures values in the verifyNSignatures function.

Consider if this optionality of using a higher threshold for signer changes should be supported for the EVM version. If so then the validation and logic in verifyNSignatures should be refactored to allow greater than or equal to the threshold of signatures to be provided.

USDT0 Team: Acknowledged.

The external call made in the executeTransaction function uses a .call invocation which automatically loads in the returnData into memory, regardless of if the returnData parameter is named in the returned tuple unpacking.

This is currently a waste of gas since the returned data is unused, and could potentially be a source of gas griefing for the executor in the case that the external call is made to an untrusted contract which may return a large amount of bytes unexpectedly.

Consider implementing a low level assembly call and specifying 0 as the length of return data to copy into memory.

USDT0 Team: Acknowledged.

The current implementation uses ECDSA.recover to verify signers, a method that only supports externally owned accounts (EOAs) signatures. As a result, any contract-based address cannot produce valid signatures under this scheme, effectively excluding multi signature wallets.

multi signature wallets and contract-based managers are incapable of directly signing in the same manner as EOAs, preventing them from providing their signatures to the OneSig contract.

This exclusion narrows the potential user base by disallowing participation from important categories such as DAO-managed treasuries or advanced contract-driven accounts, which are common in decentralized ecosystems.

Consider using the SignatureChecker library instead of ECDSA for signature verification. OpenZeppelin’s SignatureChecker.isValidSignatureNow supports both EOAs and contract wallets.

This approach allows both types of accounts to produce valid signatures, enabling a broader range of automated and contract-based use cases without excluding any part of the user base.

USDT0 Team: Acknowledged.

The OneSig’s receive function currently allows native assets to be sent directly to the contract. While the receive function successfully collects and holds the native assets, there is no corresponding event to track deposits.

Consider emitting an event within the receive function that captures essential information, such as msg.sender, the msg.value and potentially the timestamp.

USDT0 Team: Acknowledged.

The expiration time is associated with the signatures for the merkle root, hence the expiry applies to all transactions equally. However, transactions within the tree can be intended for different chains, and chains do not have a perfectly synchronized block.timestamp across them.

When if (block.timestamp > _expiry) revert MerkleRootExpired(); is validated upon transaction execution, situations may arise where a transaction is meant to be expired on all chains, but is still executable on a subset of chains.

Be aware of this risk.

USDT0 Team: Acknowledged.

The executeTransaction function uses low-level calls (e.g., address.call{ value: ... }(data)) without checking whether the target to address is a deployed smart contract that actually implements the function being called.

As a result, sending a call to an Externally Owned Account (EOA) or a non-existent contract would still succeed, even though the call effectively does nothing.

This can create a misleading indication of success, similar to historical issues (e.g. in Solady’s safeTransfer library), where the transaction appears to execute successfully but no real action took place.

Consider extending the Call struct to include a bool isSmartContract field that signals whether the target should contain code. During execution, if isSmartContract is set to true, validate that to.code.length > 0 (available in Solidity 0.8.18+).

If the address lacks code, revert the transaction. Conversely, if isSmartContract is false, allow the call to proceed.

USDT0 Team: Acknowledged.

Currently function executeTransaction only increments the nonce if the transaction call was successful. This poses an issue for all transactions encoded with a future nonce, in anticipation of the prior execution going through successfully.

If a single transaction is unable to be executed, all future transactions (leaves) must be updated and the merkle root must be updated so those following transactions would be executed.

Consider incrementing the nonce even if the transaction call was unsuccessful. However, it is important to take caution with this approach since arbitrary users can call executeTransaction.

An arbitrary user may not send enough value, cause the transaction to fail purposefully, and then allow all the following transactions to move on in a potentially unintended state.

This may be remedied by having a set of trusted executors to call executeTransaction or adding validation for all relevant values such as msg.value or gasleft().

Furthermore, it would be important to ensure eth_estimateGas can function appropriately if executeTransaction no longer reverts on failure.

USDT0 Team: Acknowledged.

When a seed is changed to invalidate a particular merkle root there is no logic to blacklist the seed from ever being reassigned.

As a result it is possible for signers of a multisig to accidentally reinstate a seed which was previously used and inadvertently reinstate a previously revoked merkle root.

Consider tracking a blacklist of past seeds so that they cannot be accidentally re-instated. Otherwise document this risk to users.

USDT0 Team: Acknowledged.

The ExecutionFailed error contains the index of the call but neglects to include the nonce of the Transaction being executed. This data may be useful when debugging transaction failures or examining simulations.

Consider including the Transaction nonce in the ExecutionFailed data.

USDT0 Team: Acknowledged.

In the OneSig contract the VERSION constant value is declared and unused. The same version string is repeated in the declaration of the DOMAIN_SEPARATOR constant.

This poses a risk if the VERSION constant were to be updated without changing the DOMAIN_SEPARATOR to match.

Use the VERSION constant in the declaration of the DOMAIN_SEPARATOR value.

USDT0 Team: Acknowledged.

The DOMAIN_SEPARATOR includes the OneSig version as a field, therefore all OneSig instances that are expected to be used in unison across networks by the same signers must have the same VERSION applied.

Be aware of this requirement and document it for users.

USDT0 Team: Acknowledged.

The external call return data is not validated in the executeTransaction function which can result in silent failure of transactions which return false on failure instead of reverting.

For example, if the signers were to initiate a withdrawal of USDT which failed for some reason the executeTransaction function call would still succeed and emit the TransactionExecuted event while the USDT transfer actually failed.

Be aware of this behavior and document it for users.

USDT0 Team: Acknowledged.

The OneSig contract does not expose functionality to read the signing domain. This functionality was standardized with EIP5267 which introduces the eip712Domain function.

Consider implementing the EIP5267 defined interface for better discoverability for signers.

USDT0 Team: Acknowledged.

The OneSig and MultiSig contracts use a floating pragma version instead of a fixed pragma. This can introduce unexpected outcomes since the contracts can be compiled across multiple Solidity versions which may have slight known or unknown differences in behavior.

Remove this ambiguity by using a fixed pragma version.

USDT0 Team: Acknowledged.

The NatSpec for the verifyNSignatures function lists the ways in which the verifyNSignatures function reverts. However it does not include reverts which occur due to an invalid signature verification with the ECDSA library.

Namely, the verifyNSignatures function can also revert when the provided s value for the signature is from the top half of the range or when the recovered signer address is the zero address.

Consider including these revert scenarios in the function documentation.

USDT0 Team: Acknowledged.

The documentation in the PROTOCOL.md file indicates that seeds are used to prevent signature replay, however this cannot be the case.

Seeds must be identical across OneSig instances which are to be used in unison, otherwise a new signature for the merkle tree would have to be generated for each chain.

This results in an unwieldy use-case when the signers want to invalidate the merkle on one chain but not on all chains. The signer must first update the seed on one chain and then execute the desired transactions on all other chains.

Finally the signer must update the seed across all of the companion deployments of OneSig so they are all in unison again.

Signers can instead execute new merkles on a target chain to advance the nonce past the desired transaction to skip. This method allows for granularity on the chain and transactions which the signers desire to skip.

Without requiring that all seeds are updated across all companion OneSig deployments. The seed should then only be used to invalidate a merkle root across all companion OneSig deployments at once, and the seed should always be updated in unison and never vary across deployments.

USDT0 Team: Acknowledged.

The executeTransaction function may be called by any arbitrary address once the merkle signatures are available. An arbitrary actor may take advantage of this by controlling the ordering in which actions take place with respect to the MultiSig transaction.

For example, if the multisig aims to execute a swap with a slippage of 5%, an arbitrary user can guarantee that they are able to sandwich that swap by executing it in a multicall with their own frontrun and backrun swaps.

In another scenario, the signers of a OneSig multisig could have signed off on an important protocol altering transaction which should only be applied when the protocol is paused. The paused state could be controlled by a separate PAUSER_ROLE address which is not the OneSig multisig.

In this case an arbitrary address can execute the OneSig action before the protocol is paused and potentially cause damage this way.

Be aware of the risk involved in allowing arbitrary executors for signed transactions. If this risk should be mitigated then consider implementing a whitelisted set of executors. Otherwise be sure to clearly document this risk to users of OneSig.

USDT0 Team: Acknowledged.

In the PROTOCOL.md file there are several misleading comments or grammatical errors:

Firstly, it is mentioned “Note: Although the domain-separator parameters are static across different deployments of OneSig, the risk of replay/unintentional signing attacks is mitigated due to the use of unique seeds, unique signers, and incrementing nonces.”

But not mentioned here is that the contract address is included in the leaf encoding, which removes the ability for replay across OneSig deployments on the same chain.

The PROTOCOL.md file still mentions the ChainID which has been replaced with the OneSigId. The SignerWithAddress interfaces is misspelled as SingerWithAddress. unsigned is misspelled as unisgned on line 86.

The description for the contract entry in the leaf encoding section reads, a 32 byte identifier representing a deployment which is a copy of the description for the oneSigId entry.

Consider correcting these documentation issues.

USDT0 Team: Acknowledged.

At the contract level it is possible for companion deployments of OneSigs to have a different threshold and varying signer sets, however this may present complications from a UI/UX perspective.

For maximum simplicity and to avoid potential confusion during the transaction execution process, consider requiring on the frontend that all companion OneSig deployments have the same signer threshold and set.

USDT0 Team: Acknowledged.