Skip to content

Latest commit

 

History

History
51 lines (30 loc) · 4.48 KB

008.md

File metadata and controls

51 lines (30 loc) · 4.48 KB
Raw

Wobbly Chrome Cuckoo

Medium

In rare cases claimSignersFor() can inappropriately set the Safe threshold and cause DoS of Safe transactions

Summary

claimSignersFor() lacks a success check for adding a new owner to the safe, while assuming that all addOwnerWithThreshold() calls succeeded for the purposes of updating the Safe threshold. Therefore, if adding an owner fails silently (possible in rare cases), then the threshold will be set incorrectly and all Safe transactions will revert due to the threshold check in checkTransaction(). This will DoS Safe transactions until the threshold is updated to the correct value. The DoS could last for more than one week if the HSG owner is a timelock contract, which is common with governance protocols.

Root Cause

The root cause is that the SafeManagerLib.sol doesn't check return bools. (Note that the Safe design allows the execute() call in execTransactionFromModule() to fail silently.) Therefore, claimSignersFor() assumes that all of the owners specified were added successfully to the safe by agnostically incrementing the local newNumOwners variable. Furthermore, it also assumes that all threshold updates succeed.

claimSignersFor() calls execSafeTransactionFromHSG() to add owners, and this function doesn't check the return bool of the execTransactionFromModule() function it calls. The same issue occurs with execChangeThreshold() a few lines below the execSafeTransactionFromHSG() call.

These silent failures can result in an improper threshold set in the Safe.

Internal pre-conditions

One example of a situation in which one of the calls to add a new owner in claimSignersFor() will silently fail:

  1. An address which is disallowed as a Safe owner is given a signer hat.
  2. claimSignersFor() is called to add multiple signers, including the disallowed signer.
  3. Adding the disallowed signer to the Safe will silently fail.

External pre-conditions

No response

Attack Path

One scenario where the threshold is improperly increased:

  1. Silent failure occurs as described when adding owners to the Safe via claimSignersFor(), and since newNumOwners is incremented more than it should be, the call to _getNewThreshold() in claimSignersFor() returns a value that's too high.
  2. claimSignersFor() changes the Safe threshold. One of two possibilities occurs, but either way the resulting Safe threshold is incorrect:
    • claimSignersFor() attempts to change the threshold to improperly high value which is disallowed by the Safe, and this fails silently similarly to how adding an already existing Safe owner fails silently.
    • claimSignersFor() successfully changes the threshold to a value that's higher than intended, but is still allowed by the Safe.
  3. Signers sign a transaction and execute it on the Safe. Due to the transaction check in HSG that requires the threshold on the Safe to be identical to the intended threshold, the transaction will fail.
  4. Suppose the HSG owner is a timelock contract (very common with governance). The Safe functionality will be DoSed for the duration of the timelock, until the timelock contract can call other functions such as setThresholdConfig() as a workaround to fix the threshold. Note that timelock periods are often longer than 1 week.

Impact

DoS of Safe functionality for longer than one week.

PoC

No response

Mitigation

Check return bools to make sure that Safe module transactions were executed successfully, and take appropriate actions if they weren't.