diff --git a/src/paymasters/SponsorPaymaster.sol b/src/paymasters/SponsorPaymaster.sol index 418603e58..1992e1aae 100644 --- a/src/paymasters/SponsorPaymaster.sol +++ b/src/paymasters/SponsorPaymaster.sol @@ -31,7 +31,7 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen uint256 public constant RATE_LIMIT_PERIOD = 1 minutes; uint256 public constant RATE_LIMIT_THRESHOLD_TOTAL = 50; - uint256 public userOpMaxCost = 0.03 ether; + uint256 public userOpMaxCost; IKintoAppRegistry public override appRegistry; mapping(address => uint256) public balances; @@ -64,6 +64,8 @@ contract SponsorPaymaster is Initializable, BasePaymaster, UUPSUpgradeable, Reen function initialize(address _owner) external virtual initializer { __UUPSUpgradeable_init(); _transferOwnership(_owner); + + userOpMaxCost = 0.03 ether; // unlocks owner unlockBlock[_owner] = block.number; } diff --git a/src/wallet/KintoWallet.sol b/src/wallet/KintoWallet.sol index 9148029ef..e99c22911 100644 --- a/src/wallet/KintoWallet.sol +++ b/src/wallet/KintoWallet.sol @@ -387,26 +387,28 @@ contract KintoWallet is Initializable, BaseAccount, TokenCallbackHandler, IKinto } function _resetSigners(address[] calldata newSigners, uint8 _policy) internal { - require(newSigners.length > 0 && newSigners.length <= MAX_SIGNERS, "KW-rs: invalid array"); + require(newSigners.length > 0 && newSigners.length <= MAX_SIGNERS, "KW-rs: signers exceed max limit"); require(newSigners[0] != address(0) && kintoID.isKYC(newSigners[0]), "KW-rs: KYC Required"); - require( - newSigners.length == 1 || (newSigners.length == 2 && newSigners[0] != newSigners[1]) - || ( - newSigners.length == 3 && (newSigners[0] != newSigners[1]) && (newSigners[1] != newSigners[2]) - && newSigners[0] != newSigners[2] - ), - "duplicate owners" - ); + + // ensure no duplicate signers + for (uint256 i = 0; i < newSigners.length; i++) { + for (uint256 j = i + 1; j < newSigners.length; j++) { + require(newSigners[i] != newSigners[j], "KW-rs: duplicate signers"); + } + } + + // ensure all signers are valid for (uint256 i = 0; i < newSigners.length; i++) { require(newSigners[i] != address(0), "KW-rs: invalid signer address"); } + + emit SignersChanged(owners, newSigners); owners = newSigners; - emit SignersChanged(newSigners, owners); - // Change policy if needed. + + // change policy if needed + require(_policy == 1 || newSigners.length > 1, "KW-rs: invalid policy for single signer"); if (_policy != signerPolicy) { setSignerPolicy(_policy); - } else { - require(_policy == 1 || newSigners.length > 1, "KW-rs: invalid policy"); } } diff --git a/src/wallet/KintoWalletFactory.sol b/src/wallet/KintoWalletFactory.sol index 5d461a4ee..117288e02 100644 --- a/src/wallet/KintoWalletFactory.sol +++ b/src/wallet/KintoWalletFactory.sol @@ -264,18 +264,18 @@ contract KintoWalletFactory is Initializable, UUPSUpgradeable, OwnableUpgradeabl (newImplementation); } - function _preventCreationBytecode(bytes calldata _bytes) internal view { - // Prevent direct deployment of KintoWallet contracts + function _preventWalletDeployment(bytes calldata _bytecode) internal view { bytes memory walletInitCode = type(SafeBeaconProxy).creationCode; - if (_bytes.length > walletInitCode.length + 32) { - // Make sure this beacon cannot be called with creation code + if (_bytecode.length > walletInitCode.length + 32) { uint256 offset = 12 + walletInitCode.length; - address beaconAddress = address(0); - bytes memory slice = _bytes[offset:offset + 20]; + + // extract beacon address directly from calldata + address beaconAddress; + bytes memory slice = _bytecode[offset:offset + 20]; assembly { beaconAddress := mload(add(slice, 20)) } - // Compare the extracted beacon address with the disallowed address + require(beaconAddress != address(beacon), "Direct KintoWallet deployment not allowed"); } } @@ -284,10 +284,14 @@ contract KintoWalletFactory is Initializable, UUPSUpgradeable, OwnableUpgradeabl internal returns (address) { - require(amount == msg.value, "amount mismatch"); - _preventCreationBytecode(bytecode); + require(amount == msg.value, "Amount mismatch"); + require(bytecode.length > 0, "Bytecode is empty"); + _preventWalletDeployment(bytecode); + + // deploy the contract using `CREATE2` address created = Create2.deploy(amount, salt, bytecode); - // Assign ownership to the deployer if needed + + // assign ownership to newOwner if the contract is Ownable try OwnableUpgradeable(created).owner() returns (address owner) { if (owner == address(this)) { OwnableUpgradeable(created).transferOwnership(newOwner); diff --git a/test/KintoWalletFactory.t.sol b/test/KintoWalletFactory.t.sol index 1d270b6c7..471346054 100644 --- a/test/KintoWalletFactory.t.sol +++ b/test/KintoWalletFactory.t.sol @@ -165,7 +165,26 @@ contract KintoWalletFactoryTest is SharedSetup { assertEq(Counter(created).count(), 1); } - function testDeployContract_RevertWhen_CreateWalletThroughDeploy() public { + function testDeployContract_RevertWhen_SenderNotKYCd() public { + vm.prank(_user2); + vm.expectRevert("KYC required"); + _walletFactory.deployContract(_owner, 0, abi.encodePacked(type(Counter).creationCode), bytes32(0)); + } + + function testDeployContract_RevertWhen_AmountMismatch() public { + vm.deal(_owner, 1 ether); + vm.prank(_owner); + vm.expectRevert("Amount mismatch"); + _walletFactory.deployContract(_owner, 1 ether + 1, abi.encodePacked(type(Counter).creationCode), bytes32(0)); + } + + function testDeployContract_RevertWhen_ZeroBytecode() public { + vm.expectRevert("Bytecode is empty"); + vm.prank(_owner); + _walletFactory.deployContract(_owner, 0, bytes(""), bytes32(0)); + } + + function testDeployContract_RevertWhen_CreateWallet() public { bytes memory initialize = abi.encodeWithSelector(IKintoWallet.initialize.selector, _owner, _owner); bytes memory bytecode = abi.encodePacked( type(SafeBeaconProxy).creationCode, abi.encode(address(_walletFactory.beacon()), initialize) @@ -175,12 +194,6 @@ contract KintoWalletFactoryTest is SharedSetup { _walletFactory.deployContract(_owner, 0, bytecode, bytes32(0)); } - function testDeployContract_RevertWhen_SenderNotKYCd() public { - vm.prank(_user2); - vm.expectRevert("KYC required"); - _walletFactory.deployContract(_owner, 0, abi.encodePacked(type(Counter).creationCode), bytes32(0)); - } - function testFundWallet() public { vm.prank(_owner); _walletFactory.fundWallet{value: 1e18}(payable(address(_kintoWallet))); diff --git a/test/SponsorPaymaster.t.sol b/test/SponsorPaymaster.t.sol index ba7db3165..a71c02743 100644 --- a/test/SponsorPaymaster.t.sol +++ b/test/SponsorPaymaster.t.sol @@ -34,6 +34,7 @@ contract SponsorPaymasterTest is SharedSetup { function testUp() public override { super.testUp(); assertEq(_paymaster.COST_OF_POST(), 200_000); + assertEq(_paymaster.userOpMaxCost(), 0.03 ether); } /* ============ Events ============ */ diff --git a/test/wallet/policy/Policy.t.sol b/test/wallet/policy/Policy.t.sol index 985d21fcf..c4713f3d9 100644 --- a/test/wallet/policy/Policy.t.sol +++ b/test/wallet/policy/Policy.t.sol @@ -7,7 +7,7 @@ import "../../SharedSetup.t.sol"; contract ResetSignerTest is SharedSetup { /* ============ Signers & Policy tests ============ */ - function testAddingOneSigner() public { + function testResetSigners_WhenAddingOneSigner() public { vm.startPrank(_owner); address[] memory owners = new address[](2); owners[0] = _owner; @@ -29,7 +29,7 @@ contract ResetSignerTest is SharedSetup { vm.stopPrank(); } - function test_RevertWhen_DuplicateSigner() public { + function testResetSigners_RevertWhen_DuplicateSigner() public { address[] memory owners = new address[](2); owners[0] = _owner; owners[1] = _owner; @@ -52,10 +52,10 @@ contract ResetSignerTest is SharedSetup { ); vm.recordLogs(); _entryPoint.handleOps(userOps, payable(_owner)); - assertRevertReasonEq("duplicate owners"); + assertRevertReasonEq("KW-rs: duplicate signers"); } - function test_RevertWhen_WithEmptyArray() public { + function testResetSigners_RevertWhen_EmptyArray() public { address[] memory owners = new address[](0); UserOperation memory userOp = _createUserOperation( @@ -79,7 +79,7 @@ contract ResetSignerTest is SharedSetup { // fixme: assertRevertReasonEq(stdError.indexOOBError)F; } - function test_RevertWhen_WithManyOwners() public { + function testResetSigners_RevertWhen_ManyOwners() public { address[] memory owners = new address[](4); owners[0] = _owner; owners[1] = _user; @@ -104,10 +104,10 @@ contract ResetSignerTest is SharedSetup { ); vm.recordLogs(); _entryPoint.handleOps(userOps, payable(_owner)); - assertRevertReasonEq("KW-rs: invalid array"); + assertRevertReasonEq("KW-rs: signers exceed max limit"); } - function test_RevertWhen_WithoutKYCSigner() public { + function testResetSigners_RevertWhen_WithoutKYCSigner() public { address[] memory owners = new address[](1); owners[0] = _user; @@ -125,7 +125,7 @@ contract ResetSignerTest is SharedSetup { _entryPoint.handleOps(userOps, payable(_owner)); } - function testChangingPolicyWithTwoSigners() public { + function testResetSigners_WhenChangingPolicy_WhenTwoSigners() public { address[] memory owners = new address[](2); owners[0] = _owner; owners[1] = _user; @@ -148,7 +148,7 @@ contract ResetSignerTest is SharedSetup { assertEq(_kintoWallet.signerPolicy(), _kintoWallet.ALL_SIGNERS()); } - function testChangingPolicyWithThreeSigners() public { + function testResetSigners_WhenChangingPolicy_WhenThreeSigners() public { vm.startPrank(_owner); address[] memory owners = new address[](3); owners[0] = _owner; @@ -173,7 +173,7 @@ contract ResetSignerTest is SharedSetup { } // todo: separate into 2 different tests - function test_RevertWhen_ChangingPolicyWithoutRightSigners() public { + function testResetSigners_RevertWhen_ChangingPolicy_WhenNotRightSigners() public { address[] memory owners = new address[](2); owners[0] = _owner; owners[1] = _user; diff --git a/test/wallet/validateSignature/ValidateSignature.t.sol b/test/wallet/validateSignature/ValidateSignature.t.sol index d267ad68e..569f610a4 100644 --- a/test/wallet/validateSignature/ValidateSignature.t.sol +++ b/test/wallet/validateSignature/ValidateSignature.t.sol @@ -810,4 +810,39 @@ contract ValidateSignatureTest is SharedSetup { ) ); } + + // special case 2: requiredSigners == 3, owners.length == 3 and only owners[0] is KYCd + function testValidateSignature_SpecialCase3() public { + assertEq(_kintoID.isKYC(_user), false); + assertEq(_kintoID.isKYC(_user2), false); + + // reset signers & change policy + address[] memory owners = new address[](3); + owners[0] = _owner; + owners[1] = _user; + owners[2] = _user2; + resetSigners(owners, _kintoWallet.ALL_SIGNERS()); + + // create user op with owners 1 as signer + privateKeys = new uint256[](3); + privateKeys[0] = _ownerPk; + privateKeys[1] = _userPk; + privateKeys[2] = _user2Pk; + + UserOperation memory userOp = _createUserOperation( + address(_kintoWallet), + address(counter), + _kintoWallet.getNonce(), + privateKeys, + abi.encodeWithSignature("increment()"), + address(_paymaster) + ); + + assertEq( + SIG_VALIDATION_FAILED, + KintoWalletHarness(payable(address(_kintoWallet))).exposed_validateSignature( + userOp, _entryPoint.getUserOpHash(userOp) + ) + ); + } }