Invalid confirmation sneaky transaction

[joshuahannan]

Problem: A transaction can be executed with an invalid number and/or set of confirmations
It is possible to execute a major or admin level transaction with a number of confirmations that is smaller than the corresponding required number of confirmations, or even with confirmations made by deprecated owners.
To demonstrate, consider that an owner calls the function serveTx with an ether value that is larger than the current majorThreshold for ether. Assuming that no previous pending identical transaction exists, a new Transaction struct will be created with a confirmRequired member set to the appropriate requiredMajor value of the wallet.
If such a Transaction, while awaiting for confirmations, remains pending for execution while a successful call and execution of changeRequiredMajor occurs, the stored Transaction struct’s confirmRequired will be out of sync with the wallet’s new requiredMajor value.
Similarly, when a confirmation occurs, the confirming owner’s address is stored in the Transaction’s confirmedOwners array and the confirmCount value is incremented. If a call and later execution of removeOwner, targeted at the owner that signed the initial transaction occurs, the associated confirmation or signature will incorrectly remain valid, and the confirm count will not be decremented. As a result, deprecated signatures will still be counted as valid signatures for the execution of a pending Transaction.
The situation described above does not only affect the serveTx function, but all the state changing operations that the wallet can perform, which require a given number of confirmations, such as WalletAdminLib's admin operations.
The problem narrows down to the fact that confirmation information is stored per-transaction instead of per-wallet. A better approach would be to store confirmation addresses only, and, at the time of execution of the transaction, iterate through the current set of active owners, and use these addresses to count the number of active owners that approved a given transaction. With such an approach, the number of confirmations required for a transaction to be executed will always correspond to the currentrequiredMinor, requiredMajor or requiredAdmin values, and only currently active owner confirmations will be considered as proper confirmations.

Solution
Consider removing confirmCount from the Transaction struct. Change confirmedOwners to mapping(address => bool) instead of uint256[]. When the time comes to verify if a transaction has enough confirmations, iterate through the owners array and count the number of owners that have a value of true in this mapping. This solution would also remove the Array256Lib dependency, reducing the wallet’s attack surface. Additionally, without the need for Array256Lib, confirmedOwners would’t have to be casted to uint256 which can also be confusing.