diff --git a/contracts/utils/cryptography/signers/MultiSignerERC7913Weighted.sol b/contracts/utils/cryptography/signers/MultiSignerERC7913Weighted.sol index faa6b926e..2d377a11b 100644 --- a/contracts/utils/cryptography/signers/MultiSignerERC7913Weighted.sol +++ b/contracts/utils/cryptography/signers/MultiSignerERC7913Weighted.sol @@ -130,6 +130,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}. * diff --git a/test/account/AccountMultiSignerWeighted.test.js b/test/account/AccountMultiSignerWeighted.test.js index 0d78f3eca..5519d8a59 100644 --- a/test/account/AccountMultiSignerWeighted.test.js +++ b/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); + }); }); });