cccz
high
reconcileSignerCount should use newThreshold instead of validSignerCount to call safe.changeThreshold.
In reconcileSignerCount, safe.changeThreshold is called when newThreshold > 0, but it incorrectly uses validSignerCount instead of newThreshold for its argument.
When validSignerCount <= target && validSignerCount ! = currentThreshold
, newThreshold == validSignerCoun.
However, when validSignerCount > target && currentThreshold < target
, newThreshold == target < validSignerCount.
signerCount = validSignerCount;
uint256 currentThreshold = safe.getThreshold();
uint256 newThreshold;
uint256 target = targetThreshold; // save SLOADs
if (validSignerCount <= target && validSignerCount != currentThreshold) {
newThreshold = validSignerCount;
} else if (validSignerCount > target && currentThreshold < target) {
newThreshold = target;
}
if (newThreshold > 0) {
bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);
Consider the current validSignerCount = 6, targetThreshold = 5, safe'threshold= 5, After alice and bob lose their signer eligibility, reconcileSignerCount is called, validSignerCount = 4, targetThreshold = 5, safe'threshold = 4. After alice and bob regain signer eligibility, reconcileSignerCount is called, validSignerCount = 6, targetThreshold = 5, safe'threshold should be 5 but is incorrectly changed to 6.
This makes safe'threshold large, so that safe'threshold exceeds targetThreshold, and more signatures are requested in the checkTransaction.
uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);
// revert if there aren't enough valid signatures
if (validSigCount < safe.getThreshold() || validSigCount < minThreshold) {
revert InvalidSigners();
}
Then in checkAfterExecution, it will revert at the following code
if (safe.getThreshold() != _getCorrectThreshold()) {
revert SignersCannotChangeThreshold();
}
Manual Review
Change to
if (validSignerCount <= target && validSignerCount != currentThreshold) {
newThreshold = validSignerCount;
} else if (validSignerCount > target && currentThreshold < target) {
newThreshold = target;
}
if (newThreshold > 0) {
- bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);
+ bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);