From 5d5458b810464f15b045308a0ece479fa691b98f Mon Sep 17 00:00:00 2001 From: excaliborr <124819095+excaliborr@users.noreply.github.com> Date: Tue, 28 Nov 2023 08:08:49 -0500 Subject: [PATCH] test: e2e verification (#25) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 🤖 Linear Closes SAF-XXX --------- Co-authored-by: OneTony --- solidity/contracts/VerifierModule.sol | 24 +++- solidity/test/e2e/VerifierModule.t.sol | 178 ++++++++++++++++++++++-- solidity/test/unit/VerifierModule.t.sol | 50 ++++++- 3 files changed, 231 insertions(+), 21 deletions(-) diff --git a/solidity/contracts/VerifierModule.sol b/solidity/contracts/VerifierModule.sol index 89481cd..76b6ce4 100644 --- a/solidity/contracts/VerifierModule.sol +++ b/solidity/contracts/VerifierModule.sol @@ -228,6 +228,18 @@ contract VerifierModule is IVerifierModule { address[] memory _newOwners = _proposedSettings.owners; bool _hasUpdatedThreshold; + // If all we need to do is a swap the rest of the logic is not needed and we can just call the swapOwner function + if (_oldOwners.length == 1 && _newOwners.length == 1 && _oldOwners[0] != _newOwners[0]) { + ISafe(_safe).execTransactionFromModule( + _safe, + 0, + abi.encodeWithSelector(ISafe.swapOwner.selector, _SENTINEL_OWNERS, _oldOwners[0], _newOwners[0]), + Enum.Operation.Call + ); + + return; + } + // NOTE: Threshold is automatically updated inside these calls if it needs to be updated for (uint256 _i; _i < _newOwners.length;) { if (!ISafe(_safe).isOwner(_newOwners[_i])) { @@ -245,16 +257,20 @@ contract VerifierModule is IVerifierModule { } } - for (uint256 _i; _i < _oldOwners.length;) { - if (!_linearSearchOwners(_oldOwners[_i], _newOwners)) { + // Get the owners again in case any updates were made from the previous loop + // We need to do this because the linked list for owners in safe inserts to the beginning of the list + address[] memory _changedOwners = ISafe(_safe).getOwners(); + + for (uint256 _i; _i < _changedOwners.length;) { + if (!_linearSearchOwners(_changedOwners[_i], _newOwners)) { _hasUpdatedThreshold = true; ISafe(_safe).execTransactionFromModule( _safe, 0, abi.encodeWithSelector( ISafe.removeOwner.selector, - _oldOwners[_i], - int256(_i) - 1 < 0 ? _SENTINEL_OWNERS : _oldOwners[_i - 1], + int256(_i) - 1 < 0 ? _SENTINEL_OWNERS : _changedOwners[_i - 1], + _changedOwners[_i], _newThreshold ), Enum.Operation.Call diff --git a/solidity/test/e2e/VerifierModule.t.sol b/solidity/test/e2e/VerifierModule.t.sol index cbdacb2..4d5b0b0 100644 --- a/solidity/test/e2e/VerifierModule.t.sol +++ b/solidity/test/e2e/VerifierModule.t.sol @@ -14,6 +14,19 @@ contract VerifierModuleE2E is CommonE2EBase { using RLPReader for RLPReader.RLPItem; using RLPReader for bytes; + IVerifierModule.SafeTxnParams _txn = IVerifierModule.SafeTxnParams({ + to: address(0), + value: 1, + data: '', + operation: Enum.Operation.Call, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: '' + }); + function setUp() public override { super.setUp(); @@ -36,6 +49,143 @@ contract VerifierModuleE2E is CommonE2EBase { assertEq(_blockNumber, _expectedBlockNumber); } + function testProposeAndVerify() public { + // Add an owner so the settings dont match the home chain + vm.prank(address(nonHomeChainSafe)); + nonHomeChainSafe.addOwnerWithThreshold(address(_searcher), 1); + + address[] memory _fakeOwners = nonHomeChainSafe.getOwners(); + + assertEq(_fakeOwners.length, 2, 'Owners should be 2'); + + bytes memory _encodedTxn = nonHomeChainSafe.encodeTransactionData( + address(0), 1, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), nonHomeChainSafe.nonce() + ); + + (uint8 _v, bytes32 _r, bytes32 _s) = vm.sign(vm.envUint('MAINNET_DEPLOYER_PK'), keccak256(_encodedTxn)); + _txn.signatures = abi.encodePacked(_r, _s, _v); + + IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1}); + + (bytes memory _storageProof, bytes memory _accountProof, bytes memory _blockHeader) = getProof(); + + StateVerifier.BlockHeader memory _blockHeaderStruct = StateVerifier.verifyBlockHeader(_blockHeader); + + oracle.updateBlockHeader(_blockHeader, _blockHeaderStruct.timestamp, _blockHeaderStruct.number); + + storageMirrorRootRegistry.proposeAndVerifyStorageMirrorStorageRoot(_accountProof); + + uint256 _searcherBalance = address(_searcher).balance; + uint256 _addressZeroBalance = address(0).balance; + + vm.prank(_searcher); + verifierModule.proposeAndVerifyUpdate(address(nonHomeChainSafe), _safeSettings, _storageProof, _txn); + + address[] memory _newOwners = nonHomeChainSafe.getOwners(); + + assertEq(_newOwners.length, 1, 'Owners should be 1'); + assertEq(_newOwners[0], address(_deployer), 'Owner should be the deployer'); + assertGt(address(_searcher).balance, _searcherBalance, 'Searcher should be rewarded'); + assertGt( + address(0).balance, + _addressZeroBalance, + 'Address zero should have more funds because we sent 1 wei the arbitrary txn' + ); + } + + function testChangingOwnersAndThreshold() public { + // Add an owner so the settings dont match the home chain + vm.startPrank(address(nonHomeChainSafe)); + nonHomeChainSafe.addOwnerWithThreshold(address(_searcher), 1); + nonHomeChainSafe.changeThreshold(2); + vm.stopPrank(); + + address[] memory _fakeOwners = nonHomeChainSafe.getOwners(); + + assertEq(_fakeOwners.length, 2, 'Owners should be 2'); + assertEq(nonHomeChainSafe.getThreshold(), 2, 'Threshold should be 2'); + + bytes memory _encodedTxn = nonHomeChainSafe.encodeTransactionData( + address(0), 1, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), nonHomeChainSafe.nonce() + ); + + (uint8 _v, bytes32 _r, bytes32 _s) = vm.sign(vm.envUint('MAINNET_DEPLOYER_PK'), keccak256(_encodedTxn)); + _txn.signatures = abi.encodePacked(_r, _s, _v); + + IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1}); + + (bytes memory _storageProof, bytes memory _accountProof, bytes memory _blockHeader) = getProof(); + + StateVerifier.BlockHeader memory _blockHeaderStruct = StateVerifier.verifyBlockHeader(_blockHeader); + + oracle.updateBlockHeader(_blockHeader, _blockHeaderStruct.timestamp, _blockHeaderStruct.number); + + storageMirrorRootRegistry.proposeAndVerifyStorageMirrorStorageRoot(_accountProof); + + uint256 _searcherBalance = address(_searcher).balance; + uint256 _addressZeroBalance = address(0).balance; + + vm.prank(_searcher); + verifierModule.proposeAndVerifyUpdate(address(nonHomeChainSafe), _safeSettings, _storageProof, _txn); + + address[] memory _newOwners = nonHomeChainSafe.getOwners(); + + assertEq(_newOwners.length, 1, 'Owners should be 1'); + assertEq(_newOwners[0], address(_deployer), 'Owner should be the deployer'); + assertGt(address(_searcher).balance, _searcherBalance, 'Searcher should be rewarded'); + assertGt( + address(0).balance, + _addressZeroBalance, + 'Address zero should have more funds because we sent 1 wei the arbitrary txn' + ); + } + + function testVerifySwappingOwners() public { + // Add an owner so the settings dont match the home chain + vm.startPrank(address(nonHomeChainSafe)); + nonHomeChainSafe.addOwnerWithThreshold(address(_searcher), 1); + nonHomeChainSafe.removeOwner(address(_searcher), address(_deployer), 1); + vm.stopPrank(); + + address[] memory _fakeOwners = nonHomeChainSafe.getOwners(); + + assertEq(_fakeOwners.length, 1, 'Owners should be 1'); + + bytes memory _encodedTxn = nonHomeChainSafe.encodeTransactionData( + address(0), 1, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), nonHomeChainSafe.nonce() + ); + + (uint8 _v, bytes32 _r, bytes32 _s) = vm.sign(vm.envUint('MAINNET_DEPLOYER_PK'), keccak256(_encodedTxn)); + _txn.signatures = abi.encodePacked(_r, _s, _v); + + IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1}); + + (bytes memory _storageProof, bytes memory _accountProof, bytes memory _blockHeader) = getProof(); + + StateVerifier.BlockHeader memory _blockHeaderStruct = StateVerifier.verifyBlockHeader(_blockHeader); + + oracle.updateBlockHeader(_blockHeader, _blockHeaderStruct.timestamp, _blockHeaderStruct.number); + + storageMirrorRootRegistry.proposeAndVerifyStorageMirrorStorageRoot(_accountProof); + + uint256 _searcherBalance = address(_searcher).balance; + uint256 _addressZeroBalance = address(0).balance; + + vm.prank(_searcher); + verifierModule.proposeAndVerifyUpdate(address(nonHomeChainSafe), _safeSettings, _storageProof, _txn); + + address[] memory _newOwners = nonHomeChainSafe.getOwners(); + + assertEq(_newOwners.length, 1, 'Owners should be 1'); + assertEq(_newOwners[0], address(_deployer), 'Owner should be the deployer'); + assertGt(address(_searcher).balance, _searcherBalance, 'Searcher should be rewarded'); + assertGt( + address(0).balance, + _addressZeroBalance, + 'Address zero should have more funds because we sent 1 wei the arbitrary txn' + ); + } + function testStorageProofIsValid() public { (bytes memory _storageProof, bytes memory _accountProof, bytes memory _blockHeader) = getProof(); @@ -70,19 +220,6 @@ contract VerifierModuleE2E is CommonE2EBase { assertEq(_fakeOwners.length, 2, 'Owners should be 2'); - IVerifierModule.SafeTxnParams memory _txn = IVerifierModule.SafeTxnParams({ - to: address(0), - value: 1, - data: '', - operation: Enum.Operation.Call, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: '' - }); - bytes memory _encodedTxn = nonHomeChainSafe.encodeTransactionData( address(0), 1, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), nonHomeChainSafe.nonce() ); @@ -101,8 +238,23 @@ contract VerifierModuleE2E is CommonE2EBase { assertEq(oracle.blockHeader(), _blockHeader, 'Block header should be saved'); assertEq(oracle.blockTimestamp(), _blockHeaderStruct.timestamp, 'Timestamp should be saved'); + uint256 _searcherBalance = address(_searcher).balance; + uint256 _addressZeroBalance = address(0).balance; + + vm.prank(_searcher); verifierModule.extractStorageRootAndVerifyUpdate( address(nonHomeChainSafe), _safeSettings, _accountProof, _storageProof, _txn ); + + address[] memory _newOwners = nonHomeChainSafe.getOwners(); + + assertEq(_newOwners.length, 1, 'Owners should be 1'); + assertEq(_newOwners[0], address(_deployer), 'Owner should be the deployer'); + assertGt(address(_searcher).balance, _searcherBalance, 'Searcher should be rewarded'); + assertGt( + address(0).balance, + _addressZeroBalance, + 'Address zero should have more funds because we sent 1 wei the arbitrary txn' + ); } } diff --git a/solidity/test/unit/VerifierModule.t.sol b/solidity/test/unit/VerifierModule.t.sol index d530fad..3f84441 100644 --- a/solidity/test/unit/VerifierModule.t.sol +++ b/solidity/test/unit/VerifierModule.t.sol @@ -173,6 +173,48 @@ abstract contract Base is Test { } contract UnitUpdateSettings is Base { + function testUpdateSwapsOwners() public { + address[] memory _oldOwners = new address[](1); + _oldOwners[0] = address(0x2); + + address[] memory _newOwners = new address[](1); + _newOwners[0] = address(0x3); + + IStorageMirror.SafeSettings memory _newSettings = IStorageMirror.SafeSettings({owners: _newOwners, threshold: 1}); + + vm.mockCall(_fakeSafe, abi.encodeWithSelector(ISafe.getOwners.selector), abi.encode(_oldOwners)); + + vm.mockCall(_fakeSafe, abi.encodeWithSelector(ISafe.getThreshold.selector), abi.encode(2)); + + vm.mockCall(_fakeSafe, abi.encodeWithSelector(ISafe.isOwner.selector), abi.encode(true)); + + vm.mockCall(_fakeSafe, abi.encodeWithSelector(ISafe.isOwner.selector, address(0x3)), abi.encode(false)); + + vm.mockCall( + _fakeSafe, + abi.encodeWithSelector( + ISafe.execTransactionFromModule.selector, + _fakeSafe, + 0, + abi.encodeWithSelector(ISafe.swapOwner.selector, address(0x1), address(0x2), address(0x3)), + Enum.Operation.Call + ), + abi.encode(true) + ); + vm.expectCall( + _fakeSafe, + abi.encodeWithSelector( + ISafe.execTransactionFromModule.selector, + _fakeSafe, + 0, + abi.encodeWithSelector(ISafe.swapOwner.selector, address(0x1), address(0x2), address(0x3)), + Enum.Operation.Call + ) + ); + + verifierModule.updateLatestVerifiedSettings(_fakeSafe, _newSettings); + } + function testUpdateSettingsAddsOwners() public { address[] memory _oldOwners = new address[](2); _oldOwners[0] = address(0x2); @@ -242,7 +284,7 @@ contract UnitUpdateSettings is Base { ISafe.execTransactionFromModule.selector, _fakeSafe, 0, - abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x4), address(0x3), 2), + abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x3), address(0x4), 2), Enum.Operation.Call ), abi.encode(true) @@ -253,7 +295,7 @@ contract UnitUpdateSettings is Base { ISafe.execTransactionFromModule.selector, _fakeSafe, 0, - abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x4), address(0x3), 2), + abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x3), address(0x4), 2), Enum.Operation.Call ) ); @@ -285,7 +327,7 @@ contract UnitUpdateSettings is Base { ISafe.execTransactionFromModule.selector, _fakeSafe, 0, - abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x2), address(0x1), 2), + abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x1), address(0x2), 2), Enum.Operation.Call ), abi.encode(true) @@ -296,7 +338,7 @@ contract UnitUpdateSettings is Base { ISafe.execTransactionFromModule.selector, _fakeSafe, 0, - abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x2), address(0x1), 2), + abi.encodeWithSelector(ISafe.removeOwner.selector, address(0x1), address(0x2), 2), Enum.Operation.Call ) );