MultiSigWallet Manual Review Findings

MSW-01M: Potentially Misbehaving Transaction Reset

Description:

When a transaction fails to execute, the executeTransaction function will revert its executed state back to false meaning that it can be re-invoked.

Example:

Copy
195if (_externalCall(txn.destination, txn.value, txn.data.length, txn.data))
196    emit TransactionExecuted(transactionId);
197else {
198    emit TransactionFailed(transactionId);
199    txn.executed = false;
200}

Recommendation:

This can have devastating consequences for two reasons; firstly, the result of call may change in the future and may yield false for a call that managed to conduct state changes. Secondly, a malicious user may have pre-approved a call to a particular address whose contract has not been deployed yet for example, or to an upgrade-able contract that they then adjust to conduct malicious activity on behalf of the wallet. As a result, we strongly recommend that transactions are executed in a one-off fashion and need to be re-submitted to be re-executed.

Alleviation:

The state is now not reset on an unsuccessful execution thereby alleviating this exhibit.

MSW-02M: Incorrect Confirmation Assumption

Description:

The isConfirmedBy function is meant to retrieve whether a particular signer has confirmed a transaction, however, it incorrectly assumes that all addresses have confirmed a transaction if it has been executed which is not true.

Example:

Copy
245function isConfirmedBy(uint transactionId, address signer)
246    external
247    view
248    returns (bool)
249{
250    // it must have been confirmed if it is already executed
251    if (transactions[transactionId].executed)
252        return true;
253
254    return confirmations[transactionId][signer];
255}

Recommendation:

We advise that the if clause is removed from the linked getter as it would yield true even for accounts that are not part of the signers array incorrectly so.

Alleviation:

The isConfirmedBy function now does not apply the optimization and instead directly looks up the confirmations double mapping properly.

MSW-03M: Incorrect Gas Stipend

Description:

The _externalCall function contains legacy code that hard-codes the gas stipend that should be removed for the call to consume. The value specified is incorrect as of the Berlin fork which introduced increases in gas across the board unless access-lists are specified, meaning that it can cause otherwise correct calls to fail due to f.e. *CALL instructions costing more than 700 gas to execute.

Example:

Copy
317result := call(
318    sub(gas(), 34710),   // 34710 is the value that solidity is currently emitting
319                       // It includes callGas (700) + callVeryLow (3, to pay for SUB) + 
320                       // callValueTransferGas (9000) + callNewAccountGas (25000, 
321                       // in case the destination address does not exist and needs creating)
322    destination,
323    value,
324    d,
325    dataLength,        // Size of the input (in bytes) - this is what fixes the padding problem
326    x,
327    0                  // Output is ignored, therefore the output size is zero
328)

Recommendation:

As gas costs aren't meant to be static, we strongly recommend the usage of a contract-level variable that can be updated to be subtracted from the gas stipend to the call instruction. This would render the solution future-proof.

Alleviation:

The _externalCall low level implementation was instead dropped for a direct, Solidity-style call thus alleviating this exhibit.