Skip to content

Gnosis

Jump to bottom
Daejun Park edited this page Jan 23, 2019 · 27 revisions

Critical Vulnerabilities

Reentrancy attack in execTransaction

A reentrancy vulnerability exists, since checkSignatures is called before nonce is incremented:

https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/GnosisSafe.sol#L90-L92

Here checkSignatures can call an external contract, ISignatureValidator.isValidSignature():

https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/GnosisSafe.sol#L161

which may initiate the same transaction using the same calldata, where nonce will remain the same, and thus the same txHashData will be generated, allowing the same signatures to be used to approve the same transaction multiple times.

At least, a malicious owner can execute a transaction multiple times, without the approval of the other owners, as long as gas remains. Moreover, if a benign owner calls another malicious contract for the signature validation, the malicious contract can exploit it even if he is not an owner.

Recommendation: Increment nonce before calling checkSignatures.

Attack Scenario

  1. Suppose we have a Gnosis safe managed by several owners, which controls access to an account that holds ERC20 tokens. At some point they agree to transfer X tokens from the safe to the personal account of owner 1.

Conditions required for this attack to be possible: (a). Owner 1 is a contract that uses EIP-1271 signature validation mechanism. (b). All other owners use either EIP-1271 or ECSDA signatures. (See this page for the 3 types of signature validation.)

  1. Owner 1 generates the transaction data for this transfer and ensures that allocated gas is 10x required amount to complete the transaction.

  2. Owner 1 requests signatures for this transaction from the other owners.

  3. Owner 1 registers a malicious ISignatureValidator contract into his own account, that once invoked, will call the Gnosis Safe with the same call data as long as there is enough gas, then return true.

  4. Owner 1 generates a signature for the transaction, of type EIP-1271, e.g. it will call the ISignatureValidator.

  5. Owner 1 calls the Gnosis Safe with the transaction data and all the signatures.

  6. During signature verification phase, Gnosis Safe invokes the malicious ISignatureValidator, that successfully calls the safe again with the same data, recursively, 9 more times.

  7. In the end, Owner 1 receives into his account 10X the amount of tokens approved by the other owners.

Other Vulnerabilities

ISignatureValidator gas abuse and refund

In this attack vector the account that initiated the transaction can consume large amounts of gas for free, unnoticed by other owners, and possibly receive a refund larger than the amount of gas consumed.

The attack is possible due to the following factors:

  • At the end of execTransaction a refund is emitted for the amount of gas consumed during the transaction.
  • By default refund is sent to tx.origin, e.g. the non-contract account that initiated the transaction, but this can be overridden by transaction parameter refundReceiver.
  • Refund can be sent in either ether or another token, in which case token and conversion rate are given by transaction parameters gasToken and gasPrice. If token is used, refund is sent through ERC20 function transfer.
  • The gas cap for the external call managed by this transaction is given by transaction parameter safeTxGas.
  • All transaction parameters are part of transaction signature that has to be approved by the threshold amount of owners.
  • However, the external call managed by the transaction is not the only external call performed. If a signature is verified using EIP-1271, then an external call to ISignatureValidator will be performed during checkSignatures.
  • The call to ISignatureValidator does not have a gas cap.
  • The total gas allocated for the transaction is not part of transaction signature.

Consequently, the transaction initiator can setup a malicious ISignatureValidator for his own account, that consumes a large amount of gas. Let's say almost maximum gas for a block. For transactions initiated by him, he will use EIP-1271 signature validation and will allocate enough gas to cover the cost of useful payload and of malicious ISignatureValidator. At the end of transaction he will get refunded for all the consumed gas, from the SafeContracts account. Other owners won't be aware of how much gas was consumed, because allocated gas is not part of validated signature.

Up to this point the malicious owner cannot get any financial benefit other than executing some payload for free. However, the custom refund options give room for more abuse.

One customization allows getting a refund in some different token. It is realistic to assume that gasPrice for the refund token could be higher than the market value. For example because it was not updated for a long time and gasPrice no longer reflects the actual market value of the token. At the same time owners might consider this discrepancy irrelevant for example because gas price was always small and there was no need to be precise. If the expensive ISignatureValidator is used combined with token refund and outdated gasPrice, the transaction initiator can get a refund many times larger than the Ethereum block size.

The other customization allows sending the refund to a different account than tx.origin. If it is the same account that controls ISignatureValidator actual implementation, and is not under the control of the transaction initiator, then abuse can happen without the knowledge of any owner.

The maximum gain an attacker may get from this vulnerability is block_gas_limit * (actual_token_value - gasPrice), so is likely small. That is why we don't classify it as critical vulnerability. However it is a valid attack vector.

One possible solution is to limit available gas when calling ISignatureValidator to a small predetermined value. Careful gas limits on external contract calls are a common security practive. For example When sending tokens with msg.sender.send(ethAmt), gas is automatically limited to 2300(source).

Other Issues/Recommendations

zero and precompiled contract addresses

execTransaction does not reject the case of to being the zero address 0x0, which may lead to an internal transaction to the zero address, via the following function call sequence:

Unlike a regular transaction to the zero address, which creates a new account, an internal transaction to the zero address behaves the same as other transactions to non-zero addresses, i.e., sending the ether to the zero address account (which indeed exists: https://etherscan.io/address/0x0000000000000000000000000000000000000000) and executing the code associated to it (which is empty in this case).

Although it is the users' responsibility that ensures correctness of the transaction data, it is quite possible that a certain user may not be aware of the difference between regular and internal transactions to the zero address, sending a transaction data to execTransaction with to == 0x0, expecting that it creates a new account. Since an internal transaction to the zero address mostly succeeds (note that it spends a little gas, without needing to pay the G_newaccount (25,000) gas fee since the zero-address account already exists), it may cause the ether stuck at 0x0, which could be serious when the user attaches a large amount of ether as a startup fund for the new account.

Recommendation:

  • reject execTransaction when to == address(0).

Minor Related Note:

handlePayment may send the ether to receiver:

where receiver is non-zero, since tx.origin cannot be zero. But, receiver could be still a non-owned account, especially one of the precompiled (0x1 - 0x8) contract addresses. Here receiver.send(amount) will succeed even with the small gas stipend 2300 for some precompiled contracts (at least, for 0x2, 0x3, 0x4, and 0x6). Below is the gas cost for executing each precompiled contract.

  • 0x1: ECREC: 3000
  • 0x2: SHA256: 60 + 12 * (byte-size-of-call-data)
  • 0x3: RIP160: 600 + 120 * (byte-size-of-call-data)
  • 0x4: ID: 15 + 3 * (byte-size-of-call-data)
  • 0x5: MODEXP: (complex-gas-cost-model)
  • 0x6: ECADD: 500
  • 0x7: ECMUL: 40000
  • 0x8: ECPAIRING: 100000 + ...

Missing check of contract existence

execTransaction misses the contract existence check for to, and thus it can cause lost ethers.

According to the Solidity document:

The low-level functions call, delegatecall and staticcall return true as their first return value if the called account is non-existent, as part of the design of EVM. Existence must be checked prior to calling if desired.

That is, if the user transaction is supposed to call an external contract with the ether payment value > 0, but makes a mistakes of referring to a non-existing address, the execute function silently returns true after transferring value to the non-existing account, resulting in the loss of the ethers.

However, it is understood that simply adding the contract existence check is not a solution, since there exists a use case that the user transaction is meant to send ethers to a non-contract account, in which case the existence check is not trivial.

Recommendation: Differentiate two types of user transactions, i.e., contract call transaction and send transaction, and implement the contract existence check for the contract call transaction. For the send transaction, explicitly mention this limitation in the document of execTransaction, and/or implement an existence check for regular accounts at the client side.

checkSignatures

Local validity check

checkSignatures checks only the first threshold number of signatures. Thus, the validity of the remaining signatures does not matter. Also, the sortedness of the whole signatures is not required, as long as the first threshold number of signatures are locally sorted. However, we have not found an attack exploiting this.

Another questionable behavior is in the case where there are threshold valid signatures in total, but some of them at the beginning are invalid. Currently, checkSignatures fails in this case. A potential issue for this behavior is that a bad owner intentionally sends an invalid signature to veto the transaction. He can always veto if his address is the first (the smallest) among the owners. On the other hand, a good owner is hard to veto some bad transaction if his address is the last (the lartest) among the owners. Is this intended?

No explicit check for the case 2 <= v <= 26

According to the signature encoding scheme, a signature with 2 <= v <= 26 is not valid, but the code does not have an explicit check for the case, relying on ecrecover to implicitly reject the case. It may be considered to have the explicit check for the robustness, if the additional gas cost is affordable, since we have not verified the underlying C implementation of secp256k1, and there might exist unknown zero-day vulnerabilities (especially for the unusual cases).

Exceptional behavior of isValidSignature

For the following code:

if (!ISignatureValidator(currentOwner).isValidSignature(data, contractSignature)) {

It is not clear what will happen if the external isValidSignature function does not return at all. Similarly, what if the currentOwner contract does not implement isValidSignature function at all, but have the default fallback function? (Note that the default fallback function cannot return anything.)

It depends on the bytecode behavior. If the bytecode does not reset the return memory address, it may reuse the garbage value previously returned by the lastOwner.isValidSignature, and it is exploitable.

Even if the current Solidity compiler generates the robust bytecode, a future version may not. Thus, it is required to re-verify the bytecode once the compiler version is updated.

In the current bytecode, it checks the existence of the return value using returndatasize: https://github.com/runtimeverification/verified-smart-contracts/blob/master/gnosis/generated/GnosisSafe.evm#L9704-L9721

Memory copy

In checkSignatures, the argument signatures is first loaded into the local memory from the call data (not into the stack). Then, when it calls isValidSignature, it performs the memory-to-memory copy to prepare for the contractSignature argument (part of the signatures bytes). Now, it is required in the bytecode that these two memory regions do not overlap. Otherwise the memory-to-memory copy is not sound.

It also depends on the compiler version.

signatures size limit

Considering the current max block gas limit (~8M) and the gas cost for the local memory usage (i.e., n^2/512 + 3n for n bytes), the size of signatures must be (much) less than 2^16 (i.e., 64KB).

OwnerManager

addOwnerWithThreshold

Although it is very unlikely, but if ownerCount is corrupted (possibly due to the hash collision), ownerCount++ may have the overflow, resulting in ownerCount being zero, provided that threshold == _threshold. In the case of threshold != _threshold, however, if ownerCount++ has the overflow, changeThreshold will always revert since the following two requirements cannot be satisfied at the same time, where ownerCount is zero:

        // Validate that threshold is smaller than number of owners.
        require(_threshold <= ownerCount, "Threshold cannot exceed owner count");
        // There has to be at least one Safe owner.
        require(_threshold >= 1, "Threshold needs to be greater than 0");

Gas consumption

[checkSignatures-loop-body-success-v1-owner]

gas: #gas(INITGAS, NONMEMGAS, MEMGAS)
  => #gas(INITGAS, NONMEMGAS +Int 1061, MEMGAS +Int (Cmem(BYZANTIUM, FINAL_MEM_USAGE) -Int Cmem(BYZANTIUM, MU)))
; FINAL_MEM_USAGE ==Int #memoryUsageUpdate(MU, SIGS_LOC +Int (65 *Int I +Int 65), 32)

[checkSignatures-loop-body-success-v1-not-owner]

gas: #gas(INITGAS, NONMEMGAS, MEMGAS)
  => #gas(INITGAS, NONMEMGAS +Int 6622, MEMGAS +Int (Cmem(BYZANTIUM, FINAL_MEM_USAGE) -Int Cmem(BYZANTIUM, MU)))
; FINAL_MEM_USAGE ==Int #memoryUsageUpdate(MU, SIGS_LOC +Int (65 *Int I +Int 65), 32)

[checkSignatures-loop-body-success-v_else]

gas: #gas(INITGAS, NONMEMGAS, MEMGAS)
  => #gas(INITGAS, NONMEMGAS +Int Cgascap(..) +Int 1334 -Int Cgascap(..) +Int 3496, MEMGAS +Int (Cmem(BYZANTIUM, FINAL_MEM_USAGE) -Int Cmem(BYZANTIUM, MU)))
; == #gas(INITGAS, NONMEMGAS +Int 4830, MEMGAS +Int (Cmem(BYZANTIUM, FINAL_MEM_USAGE) -Int Cmem(BYZANTIUM, MU)))
; FINAL_MEM_USAGE ==Int #memoryUsageUpdate(MU, NEXT_LOC +Int 128 , 32)

Lazy enum type check

The operation argument value must be with the range of Enum.Operation, i.e., [0,2] inclusive, and the Solidity compiler is supposed to generate the range check in the compiled bytecode. But it turns out that the range check does not appear in the execTransaction function, but it appears only inside the execute function. We have not found yet any exploit of this missing range check, but it is a potential vulnerability that needs an extra examination whenever the new bytecode is generated.

Recommendation: examine bytecode whenever the bytecode is updated.

Potential overflow if contract invariant is not met

There are several places where SafeMath is not used for the arithmetic operations.

https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/GnosisSafe.sol#L92 https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/GnosisSafe.sol#L139

https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/base/OwnerManager.sol#L62 https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/base/OwnerManager.sol#L79 https://github.com/gnosis/safe-contracts/blob/v0.1.0/contracts/base/OwnerManager.sol#L85

The following contract invariants are needed to rule out the possibility of overflow:

  • nonce is small enough to avoid overflow in nonce++.
  • threshold is small enough to avoid overflow in threshold * 65.
  • ownerCount >= 1 is small enough to avoid overflow in ownerCount++, ownerCount - 1, and ownerCount--.

In the current GnosisSafe contract, assuming the above invariants are practically reasonable considering the resource limitation (such as gas), but this assessment should be repeated whenever the contract is updated.

transaction reordering issue.

this function allows to update threshold, which introduces an usability issue similar to the ERC20 approve function issue.

the common usage scenario of this function is to add a new owner with increasing the threshold value (or keeping the value as is). it is very unlikely the case of decreasing the threshold value while adding a new owner. if there still exists such a use case, one can split the task into two transactions: adding a new owner, and decreasing threshold. that is, we have not found a reason for such a task, if any, to be executed atomically.

exploit scenario:

suppose there are five owners with threshold of three. suppose alice proposes a transaction of addOwnerWithThreshold(o1,4) and immediately bob proposes a transaction of addOwnerWithThreshold(o2,5). if the bob's transaction is approved before the alice's transaction, the final threshold will be 4, while it should be 5.

recommendation:

  • reject addOwnerWithThreshold if it tries to decrease threshold
  • have two more functions: increaseThreshold and decreaseThreshold

recommendation: removeOwner: it may be considered to check that _threshold <= threshold

Clone this wiki locally