Cause _addSigners to revert if it triggers a totalWeight overflow (#5790)

Author: Amxx

contracts/utils/cryptography/signers/MultiSignerERC7913Weighted.sol

@@ -129,6 +129,20 @@ abstract contract MultiSignerERC7913Weighted is MultiSignerERC7913 {
         _validateReachableThreshold();
     }
 
+    /**
+     * @dev See {MultiSignerERC7913-_addSigners}.
+     *
+     * In cases where {totalWeight} is almost `type(uint64).max` (due to a large `_totalExtraWeight`), adding new
+     * signers could cause the {totalWeight} computation to overflow. Adding a {totalWeight} calls after the new
+     * signers are added ensures no such overflow happens.
+     */
+    function _addSigners(bytes[] memory newSigners) internal virtual override {
+        super._addSigners(newSigners);
+
+        // This will revert if the new signers cause an overflow
+        _validateReachableThreshold();
+    }
+
     /**
      * @dev See {MultiSignerERC7913-_removeSigners}.
      *

test/account/AccountMultiSignerWeighted.test.js

@@ -6,6 +6,7 @@ const { getDomain } = require('../helpers/eip712');
 const { ERC4337Helper } = require('../helpers/erc4337');
 const { NonNativeSigner, P256SigningKey, RSASHA256SigningKey, MultiERC7913SigningKey } = require('../helpers/signers');
 const { PackedUserOperation } = require('../helpers/eip712-types');
+const { MAX_UINT64 } = require('../helpers/constants');
 
 const { shouldBehaveLikeAccountCore, shouldBehaveLikeAccountHolder } = require('./Account.behavior');
 const { shouldBehaveLikeERC1271 } = require('../utils/cryptography/ERC1271.behavior');
@@ -292,5 +293,20 @@ describe('AccountMultiSignerWeighted', function () {
         .withArgs(signer1)
         .to.not.emit(this.mock, 'ERC7913SignerWeightChanged');
     });
+
+    it('should revert if total weight to overflow (_setSignerWeights)', async function () {
+      await expect(this.mock.$_setSignerWeights([signer1, signer2, signer3], [1n, 1n, MAX_UINT64 - 1n]))
+        .to.be.revertedWithCustomError(this.mock, 'SafeCastOverflowedUintDowncast')
+        .withArgs(64, MAX_UINT64 + 1n);
+    });
+
+    it('should revert if total weight to overflow (_addSigner)', async function () {
+      await this.mock.$_setSignerWeights([signer1, signer2, signer3], [1n, 1n, MAX_UINT64 - 2n]);
+      await expect(this.mock.totalWeight()).to.eventually.equal(MAX_UINT64);
+
+      await expect(this.mock.$_addSigners([signer4]))
+        .to.be.revertedWithCustomError(this.mock, 'SafeCastOverflowedUintDowncast')
+        .withArgs(64, MAX_UINT64 + 1n);
+    });
   });
 });