diff --git a/starknet/src/authenticators/eth_tx.cairo b/starknet/src/authenticators/eth_tx.cairo index 71ba3e476..233bbea4b 100644 --- a/starknet/src/authenticators/eth_tx.cairo +++ b/starknet/src/authenticators/eth_tx.cairo @@ -69,12 +69,13 @@ mod EthTxAuthenticator { use starknet::{ContractAddress, EthAddress, Felt252TryIntoEthAddress, EthAddressIntoFelt252,}; use sx::interfaces::{ISpaceDispatcher, ISpaceDispatcherTrait}; use sx::types::{UserAddress, Strategy, IndexedStrategy, Choice}; + use sx::utils::LegacyHashFelt252EthAddress; use sx::utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR}; #[storage] struct Storage { _starknet_commit_address: EthAddress, - _commits: LegacyMap:: + _commits: LegacyMap::<(felt252, EthAddress), bool> } #[external(v0)] @@ -176,19 +177,17 @@ mod EthTxAuthenticator { assert( from_address == self._starknet_commit_address.read().into(), 'Invalid commit address' ); - // Prevents hash being overwritten by a different sender. - assert(self._commits.read(hash).into() == 0, 'Commit already exists'); - self._commits.write(hash, sender_address.try_into().unwrap()); + let sender_address = sender_address.try_into().unwrap(); + assert(self._commits.read((hash, sender_address)) == false, 'Commit already exists'); + self._commits.write((hash, sender_address), true); } #[generate_trait] impl InternalImpl of InternalTrait { fn consume_commit(ref self: ContractState, hash: felt252, sender_address: EthAddress) { - let committer_address = self._commits.read(hash); - assert(committer_address.is_non_zero(), 'Commit not found'); - assert(committer_address == sender_address, 'Invalid sender address'); + assert(self._commits.read((hash, sender_address)), 'Commit not found'); // Delete the commit to prevent replay attacks. - self._commits.write(hash, Zeroable::zero()); + self._commits.write((hash, sender_address), false); } } } diff --git a/starknet/src/lib.cairo b/starknet/src/lib.cairo index 30f89f027..3437e42a8 100644 --- a/starknet/src/lib.cairo +++ b/starknet/src/lib.cairo @@ -118,8 +118,8 @@ mod utils { mod legacy_hash; use legacy_hash::{ - LegacyHashEthAddress, LegacyHashUsedSalts, LegacyHashChoice, LegacyHashUserAddress, - LegacyHashVotePower, LegacyHashVoteRegistry, LegacyHashSpanFelt252 + LegacyHashEthAddress, LegacyHashFelt252EthAddress, LegacyHashUsedSalts, LegacyHashChoice, + LegacyHashUserAddress, LegacyHashVotePower, LegacyHashVoteRegistry, LegacyHashSpanFelt252 }; mod math; diff --git a/starknet/src/utils/legacy_hash.cairo b/starknet/src/utils/legacy_hash.cairo index 0c74cc8c2..9e5dbd5f7 100644 --- a/starknet/src/utils/legacy_hash.cairo +++ b/starknet/src/utils/legacy_hash.cairo @@ -15,6 +15,14 @@ impl LegacyHashEthAddress of LegacyHash { } } +impl LegacyHashFelt252EthAddress of LegacyHash<(felt252, EthAddress)> { + fn hash(state: felt252, value: (felt252, EthAddress)) -> felt252 { + let (_felt252, _eth_address) = value; + let state = LegacyHash::hash(state, _felt252); + LegacyHash::hash(state, _eth_address) + } +} + impl LegacyHashSpanFelt252 of LegacyHash> { fn hash(state: felt252, mut value: Span) -> felt252 { let len = value.len(); diff --git a/tests/eth-tx-auth.test.ts b/tests/eth-tx-auth.test.ts index 9c6f1ec32..f3b67a657 100644 --- a/tests/eth-tx-auth.test.ts +++ b/tests/eth-tx-auth.test.ts @@ -473,10 +473,68 @@ describe('Ethereum Transaction Authenticator', function () { ); expect.fail('Should have failed'); } catch (err: any) { - expect(err.message).to.contain(shortString.encodeShortString('Invalid sender address')); + expect(err.message).to.contain(shortString.encodeShortString('Commit not found')); } }, 1000000); + it('should not revert if the same commit was made twice', async () => { + await starknet.devnet.restart(); + await starknet.devnet.load('./dump.pkl'); + await starknet.devnet.increaseTime(10); + await starknet.devnet.loadL1MessagingContract(eth_network, mockStarknetMessaging.address); + + const proposal = { + author: signer.address, + metadataUri: ['0x1', '0x2', '0x3', '0x4'], + executionStrategy: { + address: '0x0000000000000000000000000000000000005678', + params: ['0x0'], + }, + userProposalValidationParams: [ + '0xffffffffffffffffffffffffffffffffffffffffff', + '0x1234', + '0x5678', + '0x9abc', + ], + }; + + const proposeCommitPreImage = CallData.compile({ + target: space.address, + selector: selector.getSelectorFromName('propose'), + ...proposal, + }); + + // Commit hash of payload to the Starknet Commit L1 contract + const commit = `0x${poseidonHashMany(proposeCommitPreImage.map((v) => BigInt(v))).toString( + 16, + )}`; + + // Committing payload from a different address to the author + await starknetCommit + .connect(invalidSigner) + .commit(ethTxAuthenticator.address, commit, { value: 18485000000000 }); + + expect((await starknet.devnet.flush()).consumed_messages.from_l1).to.have.a.lengthOf(1); + + // Committing the same payload from the author + await starknetCommit.commit(ethTxAuthenticator.address, commit, { value: 18485000000000 }); + + expect((await starknet.devnet.flush()).consumed_messages.from_l1).to.have.a.lengthOf(1); + + await account.invoke( + ethTxAuthenticator, + 'authenticate_propose', + CallData.compile({ + target: space.address, + author: proposal.author, + metadataURI: proposal.metadataUri, + executionStrategy: proposal.executionStrategy, + userProposalValidationParams: proposal.userProposalValidationParams, + }), + { rawInput: true }, + ); + }, 1000000); + it('a commit cannot be consumed twice', async () => { await starknet.devnet.restart(); await starknet.devnet.load('./dump.pkl');