Possible to call setup with invalid owner array

[rmeissner]

There is a bug within the setupOwners function on OwnerManager.sol, which allows duplicate owners to be onboarded when the duplicated address is next to itself in the _owners array.

This could cause a couple of unexpected behaviors and potentially cause users to lose access to their funds. It could allow the owner of the safe to set the threshold to something unreachable, potentially making it so they could not execute a safe transaction. It could also lock users out of their funds if they choose to transfer them to a CREATE2 address then set up the contract with duplicated owners and a too high threshold.

Tests

Tests can be found on https://github.com/gnosis/safe-contracts/tree/duplicate-owners

The duplicateOwners test file sets up a safe with duplicated owners. It shows that the getOwner arrays are returned with a single instance of the duplicated address and the zero address.

(The zero address appears because the owner count is higher than the number of owners in the owners mapping, so the array created in getOwners doesn’t get filled to its declared length.)

It also shows that the threshold can be maxed out so that the threshold is higher than the number of owners. All the tests provided pass but should fail on the proxy creation because of the duplicated owners.