From e66767dbcf3a4b3c3840d592440c76648507d6e9 Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Tue, 19 Nov 2024 12:45:35 +0200 Subject: [PATCH 1/8] Audit Gateway C1: Fix signers hash being able to be spoofed. --- gateway/src/auth.rs | 10 ++--- tests/gmp/gateway.test.ts | 83 ++++++++++++++++++++++++++++++++++----- tests/helpers.ts | 29 ++++++++++---- 3 files changed, 97 insertions(+), 25 deletions(-) diff --git a/gateway/src/auth.rs b/gateway/src/auth.rs index ff1f1f6..f471621 100644 --- a/gateway/src/auth.rs +++ b/gateway/src/auth.rs @@ -169,13 +169,9 @@ pub trait AuthModule: events::Events { ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); - for signer in signers.signers.iter() { - encoded.append(signer.signer.as_managed_buffer()); - encoded.append(&signer.weight.to_bytes_be_buffer()); - } - - encoded.append(&signers.threshold.to_bytes_be_buffer()); - encoded.append(signers.nonce.as_managed_buffer()); + let _ = signers + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode weighted signers")); self.crypto().keccak256(encoded) } diff --git a/tests/gmp/gateway.test.ts b/tests/gmp/gateway.test.ts index 4793399..b2bac85 100644 --- a/tests/gmp/gateway.test.ts +++ b/tests/gmp/gateway.test.ts @@ -1374,6 +1374,66 @@ describe('View functions', () => { ], }).assertFail({ code: 4, message: 'Invalid signers' }); }); + + test('Validate proof manipulate signers hash ', async () => { + await deployContract(); + + const data = e.List(); + const dataHash = getKeccak256Hash(Buffer.concat([ + Buffer.from('00', 'hex'), // ApproveMessages command type, + data.toTopU8A(), + ])); + + const spoofedWeight = 198540501017381061748170477501535016683251460902464757637395385927941042274417159n; + const signers = [ + { signer: ALICE_PUB_KEY, weight: 5n }, + { + signer: BOB_PUB_KEY, + weight: spoofedWeight, + }, + ]; + let dataForSignersHashToSpoof = Buffer.concat([ + ...signers.map(signer => { + let weightHex = signer.weight.toString(16); + if (weightHex.length % 2) { + weightHex = '0' + weightHex; + } + + return Buffer.concat([ + Buffer.from(signer.signer, 'hex'), + Buffer.from(weightHex, 'hex'), + ]); + }), + Buffer.from('0a', 'hex'), // threshold + Buffer.from(getKeccak256Hash('nonce1'), 'hex'), + ]); + const spoofedWeightedSigners = e.Tuple( + e.List( + e.Tuple(e.TopBuffer(ALICE_PUB_KEY), e.U(5)), + e.Tuple(e.TopBuffer(BOB_PUB_KEY), e.U(spoofedWeight)), + ), + e.U(10), + e.TopBuffer(getKeccak256Hash('nonce1')), + ); + + // Hash can not be the same + assert(getKeccak256Hash(dataForSignersHashToSpoof) != defaultSignersHash.toString('hex')); + + // Bob spoofed signature, but it will not work + await world.query({ + callee: contract, + funcName: 'validateProof', + funcArgs: [ + e.TopBuffer(dataHash), + generateProof( + spoofedWeightedSigners, [ + null, + generateMessageSignature(defaultSignersHash, data, './bob.pem'), + ], + ), + ], + }).assertFail({ code: 4, message: 'Invalid signers' }) + }); }); test('Approve validate execute external contract', async () => { @@ -1460,7 +1520,7 @@ test('Approve validate execute external contract', async () => { e.Str(OTHER_CHAIN_ADDRESS), payload, ], - }).assertFail({ code: 4, message: "can't withdraw before deadline" }); + }).assertFail({ code: 4, message: 'can\'t withdraw before deadline' }); await world.setCurrentBlockInfo({ timestamp: 10, @@ -1484,7 +1544,7 @@ test('Approve validate execute external contract', async () => { ...baseKvs(), // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str('1')), ], }); // Other user received funds @@ -1523,7 +1583,8 @@ const multisigSignersHash = getSignersHash( '290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563', ); -test('Approve messages with multisig prover encoded data', async () => { +// TODO: Update CosmWasm contracts +test.skip('Approve messages with multisig prover encoded data', async () => { await deployContract(); // Mock multisig signers in contract @@ -1534,7 +1595,7 @@ test('Approve messages with multisig prover encoded data', async () => { e.kvs.Mapper('signer_hash_by_epoch', e.U(1)).Value(e.TopBuffer(multisigSignersHash)), e.kvs.Mapper('epoch_by_signer_hash', e.TopBuffer(multisigSignersHash)).Value(e.U(1)), - ] + ], }); // 00000009 67616e616368652d31 - length of `ganache-1` source chain string followed by it as hex @@ -1563,8 +1624,8 @@ test('Approve messages with multisig prover encoded data', async () => { // 00 - fifth signature encoded as a None option (the fifth signer didn't specify any signature) const proof = Buffer.from( '000000050139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba0000000101ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b60000000101ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a444900000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e56300000005019543286a58ded1c031fcf8e5fcdc7c5b48b6304c539bdf7a30a0b780451a64318420fe654a13be7a33cae4f221cd26e1e033d01da144901453474c73b520450d01199b7e0f25ff4c24637bbdfdc18d338f422793f492a81140afd080019061088ddf667f018d88928a28dcb77a2c253c66ee5a83be2d4134ff3ab3141f0fdb170d014f883c316682c6e000bf4c92536a138f78c6af265f4f13d7210110e40350bb4d99e049677db13e7c12f8a4e617a5cb9bf32f5142cd58f7146505078e2d6757030000', - 'hex' - ) + 'hex', + ); await deployer.callContract({ callee: contract, @@ -1587,12 +1648,14 @@ test('Approve messages with multisig prover encoded data', async () => { e.kvs.Mapper('epoch_by_signer_hash', e.TopBuffer(multisigSignersHash)).Value(e.U(1)), // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer('b8fe5d23fe5954fa900ec37278b8661a98d061dfa34e7ebdd2e3ae98fbee5d8d')), + e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer( + 'b8fe5d23fe5954fa900ec37278b8661a98d061dfa34e7ebdd2e3ae98fbee5d8d')), ], }); }); -test('Rotate signers with multisig prover encoded data', async () => { +// TODO: Update CosmWasm contracts +test.skip('Rotate signers with multisig prover encoded data', async () => { await deployContract(); // Mock multisig signers in contract @@ -1603,7 +1666,7 @@ test('Rotate signers with multisig prover encoded data', async () => { e.kvs.Mapper('signer_hash_by_epoch', e.U(1)).Value(e.TopBuffer(multisigSignersHash)), e.kvs.Mapper('epoch_by_signer_hash', e.TopBuffer(multisigSignersHash)).Value(e.U(1)), - ] + ], }); // 00000003 - length of new signers @@ -1636,7 +1699,7 @@ test('Rotate signers with multisig prover encoded data', async () => { // 00 - fifth signature encoded as a None option (the fifth signer didn't specify any signature) const proof = Buffer.from( '000000050139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba0000000101ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b60000000101ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a444900000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e56300000005015c98a98d1e47adecf83a10d4fdc542aae1cb13ab8e6d3f5e237ad75ccb6608631c0d3f8735e3f5f481e82f088fe5215d431ae8c6abf68b96797df4bbe610cd0501ca0999eac93ee855ea88680b8094660635a06743e9acdb8d1987a9c48a60e9f794bd22a10748bb9c3c961ddc3068a96abfae00a9c38252a4b3ad99caeb06080501deca8b224a38ad99ec4cb4f3d8e86778544c55ab0c4513ce8af834b81b3e934eef29727cc76c364f316a44c2eea82fa655f209f0c5205a209461d8a7fbbacf030000', - 'hex' + 'hex', ); await world.setCurrentBlockInfo({ diff --git a/tests/helpers.ts b/tests/helpers.ts index 5aabbd1..922c03e 100644 --- a/tests/helpers.ts +++ b/tests/helpers.ts @@ -80,23 +80,23 @@ export const generateRotateSignersSignature = (signersHash: Buffer, data: Encoda }; export const getSignersHash = (signers: { signer: string, weight: number } [], threshold: number, nonce: string) => { - let thresholdHex = threshold.toString(16); - if (thresholdHex.length % 2) { - thresholdHex = '0' + thresholdHex; - } + let signersLengthHex = numberToHex(signers.length, 4); + + let thresholdHex = numberToHex(threshold); let data = Buffer.concat([ + Buffer.from(signersLengthHex, 'hex'), ...signers.map(signer => { - let weightHex = signer.weight.toString(16); - if (weightHex.length % 2) { - weightHex = '0' + weightHex; - } + let weightHex = numberToHex(signer.weight); + let weightHexLengthHex = numberToHex(weightHex.length / 2, 4); return Buffer.concat([ Buffer.from(signer.signer, 'hex'), + Buffer.from(weightHexLengthHex, 'hex'), Buffer.from(weightHex, 'hex'), ]); }), + Buffer.from(numberToHex(thresholdHex.length / 2, 4), 'hex'), Buffer.from(thresholdHex, 'hex'), Buffer.from(nonce, 'hex'), ]); @@ -104,6 +104,19 @@ export const getSignersHash = (signers: { signer: string, weight: number } [], t return createKeccakHash('keccak256').update(data).digest(); }; +const numberToHex = (nb: number, size: number = 0): string => { + let nbHex = nb.toString(16); + if (nbHex.length % 2) { + nbHex = '0' + nbHex; + } + + while (size && nbHex.length < size * 2) { + nbHex = '00' + nbHex; + } + + return nbHex; +} + export const getSignersHashAndEncodable = (signers: { signer: string, weight: number From bcdd562841dcca19a73b240117673b72ecfb125f Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Tue, 19 Nov 2024 15:22:26 +0200 Subject: [PATCH 2/8] Audit Gateway C2 & C3 & T1: Fix message hash and remove command id. --- gateway/src/constants.rs | 30 +++--- gateway/src/events.rs | 2 - gateway/src/lib.rs | 95 +++++++++--------- tests/gmp/gateway.test.ts | 97 +++++++------------ tests/governance.test.ts | 15 +-- tests/helpers.ts | 22 ++++- .../itsExecuteDeployInterchainToken.test.ts | 16 +-- .../its/itsExecuteDeployTokenManager.test.ts | 10 +- .../its/itsExecuteInterchainTransfer.test.ts | 16 +-- ...sExecuteInterchainTransferWithData.test.ts | 16 +-- tests/its/itsExpressExecute.test.ts | 4 +- tests/itsHelpers.ts | 17 +--- 12 files changed, 158 insertions(+), 182 deletions(-) diff --git a/gateway/src/constants.rs b/gateway/src/constants.rs index 06db8d2..e46be7b 100644 --- a/gateway/src/constants.rs +++ b/gateway/src/constants.rs @@ -1,7 +1,7 @@ +use core::convert::TryFrom; use multiversx_sc::api::ED25519_KEY_BYTE_LEN; use multiversx_sc::api::ED25519_SIGNATURE_BYTE_LEN; use multiversx_sc::api::KECCAK256_RESULT_LEN; -use core::convert::TryFrom; multiversx_sc::imports!(); multiversx_sc::derive_imports!(); @@ -42,6 +42,12 @@ pub struct Proof { pub signatures: ManagedVec>>, } +#[derive(TypeAbi, TopDecode, TopEncode, NestedEncode)] +pub struct CrossChainId { + pub source_chain: ManagedBuffer, + pub message_id: ManagedBuffer, +} + const MESSAGE_EXECUTED: &[u8; 1] = b"1"; #[derive(TypeAbi, PartialEq, Default)] @@ -53,32 +59,28 @@ pub enum MessageState { } impl codec::TopEncode for MessageState { - fn top_encode_or_handle_err( - &self, - output: O, - h: H, - ) -> Result<(), H::HandledErr> - where - O: codec::TopEncodeOutput, - H: codec::EncodeErrorHandler, + fn top_encode_or_handle_err(&self, output: O, h: H) -> Result<(), H::HandledErr> + where + O: codec::TopEncodeOutput, + H: codec::EncodeErrorHandler, { match self { MessageState::NonExistent => codec::TopEncode::top_encode_or_handle_err(&"", output, h), MessageState::Approved(hash) => { codec::TopEncode::top_encode_or_handle_err(hash.as_managed_buffer(), output, h) - }, + } MessageState::Executed => { codec::TopEncode::top_encode_or_handle_err(MESSAGE_EXECUTED, output, h) - }, + } } } } impl codec::TopDecode for MessageState { fn top_decode_or_handle_err(input: I, h: H) -> Result - where - I: codec::TopDecodeInput, - H: codec::DecodeErrorHandler, + where + I: codec::TopDecodeInput, + H: codec::DecodeErrorHandler, { let decoded_input = ManagedBuffer::top_decode_or_handle_err(input, h)?; if decoded_input.is_empty() { diff --git a/gateway/src/events.rs b/gateway/src/events.rs index 73049f9..6bc797f 100644 --- a/gateway/src/events.rs +++ b/gateway/src/events.rs @@ -19,7 +19,6 @@ pub trait Events { #[event("message_approved_event")] fn message_approved_event( &self, - #[indexed] command_id: &ManagedByteArray, #[indexed] source_chain: ManagedBuffer, #[indexed] message_id: ManagedBuffer, #[indexed] source_address: ManagedBuffer, @@ -30,7 +29,6 @@ pub trait Events { #[event("message_executed_event")] fn message_executed_event( &self, - #[indexed] command_id: &ManagedByteArray, #[indexed] source_chain: &ManagedBuffer, #[indexed] message_id: &ManagedBuffer, ); diff --git a/gateway/src/lib.rs b/gateway/src/lib.rs index dc09b63..5f427e0 100644 --- a/gateway/src/lib.rs +++ b/gateway/src/lib.rs @@ -74,15 +74,15 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events WeightedSigners::::top_decode(new_signers) .unwrap_or_else(|_| sc_panic!("Could not decode new signers")); - let enfore_rotation_delay = self.blockchain().get_caller() != self.operator().get(); + let enforce_rotation_delay = self.blockchain().get_caller() != self.operator().get(); let is_latest_signers = self.validate_proof(data_hash, proof); require!( - !enfore_rotation_delay || is_latest_signers, + !enforce_rotation_delay || is_latest_signers, "Not latest signers" ); - self.rotate_signers_raw(new_signers, enfore_rotation_delay); + self.rotate_signers_raw(new_signers, enforce_rotation_delay); } /// Public Methods @@ -107,28 +107,30 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events #[endpoint(validateMessage)] fn validate_message( &self, - source_chain: &ManagedBuffer, - message_id: &ManagedBuffer, + source_chain: ManagedBuffer, + message_id: ManagedBuffer, source_address: &ManagedBuffer, payload_hash: &ManagedByteArray, ) -> bool { - let command_id = self.message_to_command_id(source_chain, message_id); - - let message_hash = self.message_hash( + let cross_chain_id = CrossChainId { source_chain, message_id, + }; + + let message_hash = self.message_hash( + &cross_chain_id, source_address, &self.blockchain().get_caller(), payload_hash, ); - let messages_mapper = self.messages(&command_id); + let messages_mapper = self.messages(&cross_chain_id); let valid = messages_mapper.get() == MessageState::Approved(message_hash); if valid { messages_mapper.set(MessageState::Executed); - self.message_executed_event(&command_id, source_chain, message_id); + self.message_executed_event(&cross_chain_id.source_chain, &cross_chain_id.message_id); } valid @@ -137,17 +139,19 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events // Self Functions fn approve_message(&self, message: Message) { - let command_id = self.message_to_command_id(&message.source_chain, &message.message_id); + let cross_chain_id = CrossChainId { + source_chain: message.source_chain, + message_id: message.message_id, + }; - let messages_mapper = self.messages(&command_id); + let messages_mapper = self.messages(&cross_chain_id); if messages_mapper.get() != MessageState::NonExistent { return; } let message_hash = self.message_hash( - &message.source_chain, - &message.message_id, + &cross_chain_id, &message.source_address, &message.contract_address, &message.payload_hash, @@ -156,9 +160,8 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events messages_mapper.set(MessageState::Approved(message_hash)); self.message_approved_event( - &command_id, - message.source_chain, - message.message_id, + cross_chain_id.source_chain, + cross_chain_id.message_id, message.source_address, message.contract_address, message.payload_hash, @@ -167,17 +170,21 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events fn message_hash( &self, - source_chain: &ManagedBuffer, - message_id: &ManagedBuffer, + cross_chain_id: &CrossChainId, source_address: &ManagedBuffer, contract_address: &ManagedAddress, payload_hash: &ManagedByteArray, ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); - encoded.append(source_chain); - encoded.append(message_id); - encoded.append(source_address); + let _ = cross_chain_id + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode cross chain id")); + + let _ = source_address + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode source address")); + encoded.append(contract_address.as_managed_buffer()); encoded.append(payload_hash.as_managed_buffer()); @@ -199,58 +206,48 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events self.crypto().keccak256(encoded) } - fn message_to_command_id( - &self, - source_chain: &ManagedBuffer, - message_id: &ManagedBuffer, - ) -> ManagedByteArray { - // Axelar doesn't allow `sourceChain` to contain '_', hence this encoding is umambiguous - let mut encoded = ManagedBuffer::new(); - - encoded.append(source_chain); - encoded.append(&ManagedBuffer::from("_")); - encoded.append(message_id); - - self.crypto().keccak256(encoded) - } - #[view(isMessageApproved)] fn is_message_approved( &self, - source_chain: &ManagedBuffer, - message_id: &ManagedBuffer, + source_chain: ManagedBuffer, + message_id: ManagedBuffer, source_address: &ManagedBuffer, contract_address: &ManagedAddress, payload_hash: &ManagedByteArray, ) -> bool { - let command_id = self.message_to_command_id(source_chain, message_id); - - let message_hash = self.message_hash( + let cross_chain_id = CrossChainId { source_chain, message_id, + }; + + let message_hash = self.message_hash( + &cross_chain_id, source_address, contract_address, payload_hash, ); - self.messages(&command_id).get() == MessageState::Approved(message_hash) + self.messages(&cross_chain_id).get() == MessageState::Approved(message_hash) } #[view(isMessageExecuted)] fn is_message_executed( &self, - source_chain: &ManagedBuffer, - message_id: &ManagedBuffer, + source_chain: ManagedBuffer, + message_id: ManagedBuffer, ) -> bool { - self.messages(&self.message_to_command_id(source_chain, message_id)) - .get() - == MessageState::Executed + let cross_chain_id = CrossChainId { + source_chain, + message_id, + }; + + self.messages(&cross_chain_id).get() == MessageState::Executed } #[view(messages)] #[storage_mapper("messages")] fn messages( &self, - command_id: &ManagedByteArray, + cross_chain_id: &CrossChainId, ) -> SingleValueMapper>; } diff --git a/tests/gmp/gateway.test.ts b/tests/gmp/gateway.test.ts index b2bac85..d5ff64e 100644 --- a/tests/gmp/gateway.test.ts +++ b/tests/gmp/gateway.test.ts @@ -9,16 +9,16 @@ import { generateMessageSignature, generateProof, generateRotateSignersSignature, - getKeccak256Hash, - getSignersHash, MESSAGE_ID, - MULTISIG_PROVER_PUB_KEY_1, - MULTISIG_PROVER_PUB_KEY_2, OTHER_CHAIN_ADDRESS, OTHER_CHAIN_NAME, + getKeccak256Hash, getMessageHash, + getSignersHash, + MESSAGE_ID, + OTHER_CHAIN_ADDRESS, + OTHER_CHAIN_NAME, PAYLOAD_HASH, TOKEN_ID, TOKEN_ID2, } from '../helpers'; -import createKeccakHash from 'keccak'; -import { deployPingPongInterchain, gateway, its, mockGatewayMessageApproved, pingPong } from '../itsHelpers'; +import { deployPingPongInterchain, pingPong } from '../itsHelpers'; import { Buffer } from 'buffer'; let world: SWorld; @@ -350,7 +350,7 @@ describe('Approve messages', () => { ); const messageHash = getKeccak256Hash('mock'); - const commandId = getKeccak256Hash('ethereum_messageId'); + const crossChainId = e.Tuple(e.Str('ethereum'), e.Str('messageId')); // Mock message approved await contract.setAccount({ @@ -360,7 +360,7 @@ describe('Approve messages', () => { ...baseKvs(), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); @@ -387,8 +387,8 @@ describe('Approve messages', () => { kvs: [ ...baseKvs(), - // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + // Message was approved + e.kvs.Mapper('messages', crossChainId).Value(e.TopBuffer(messageHash)), ], }); }); @@ -404,7 +404,7 @@ describe('Approve messages', () => { e.TopBuffer(PAYLOAD_HASH), ); - const commandId = getKeccak256Hash('ethereum_messageId'); + const crossChainId = e.Tuple(e.Str('ethereum'), e.Str('messageId')); // Mock message executed await contract.setAccount({ @@ -414,7 +414,7 @@ describe('Approve messages', () => { ...baseKvs(), // Manually execute message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str('1')), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); @@ -442,7 +442,7 @@ describe('Approve messages', () => { ...baseKvs(), // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str('1')), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); }); @@ -457,16 +457,9 @@ describe('Approve messages', () => { deployer, e.TopBuffer(PAYLOAD_HASH), ); - const messageData = Buffer.concat([ - Buffer.from('ethereum'), - Buffer.from('messageId'), - Buffer.from('0x4976da71bF84D750b5451B053051158EC0A4E876'), - deployer.toTopU8A(), - Buffer.from(PAYLOAD_HASH, 'hex'), - ]); - const messageHash = getKeccak256Hash(messageData); + const messageHash = getMessageHash('ethereum', 'messageId', '0x4976da71bF84D750b5451B053051158EC0A4E876', deployer); - const commandId = getKeccak256Hash('ethereum_messageId'); + const crossChainId = e.Tuple(e.Str('ethereum'), e.Str('messageId')); await deployer.callContract({ callee: contract, @@ -489,8 +482,8 @@ describe('Approve messages', () => { kvs: [ ...baseKvs(), - // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + // Message was approved + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); }); @@ -1041,16 +1034,9 @@ describe('Validate message', () => { test('Validate message valid', async () => { await deployContract(); - const messageData = Buffer.concat([ - Buffer.from('ethereum'), - Buffer.from('messageId'), - Buffer.from('0x4976da71bF84D750b5451B053051158EC0A4E876'), - deployer.toTopU8A(), - Buffer.from(PAYLOAD_HASH, 'hex'), - ]); - const messageHash = getKeccak256Hash(messageData); + const messageHash = getMessageHash('ethereum', 'messageId', '0x4976da71bF84D750b5451B053051158EC0A4E876', deployer); - const commandId = getKeccak256Hash('ethereum_messageId'); + const crossChainId = e.Tuple(e.Str('ethereum'), e.Str('messageId')); await contract.setAccount({ ...await contract.getAccount(), @@ -1059,7 +1045,7 @@ describe('Validate message', () => { ...baseKvs(), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); @@ -1082,7 +1068,7 @@ describe('Validate message', () => { ...baseKvs(), // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str('1')), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); }); @@ -1147,19 +1133,12 @@ describe('Operator', () => { }); describe('View functions', () => { - const commandId = getKeccak256Hash('ethereum_messageId'); + const crossChainId = e.Tuple(e.Str('ethereum'), e.Str('messageId')); test('Message approved', async () => { await deployContract(); - const messageData = Buffer.concat([ - Buffer.from('ethereum'), - Buffer.from('messageId'), - Buffer.from('0x4976da71bF84D750b5451B053051158EC0A4E876'), - deployer.toTopU8A(), - Buffer.from(PAYLOAD_HASH, 'hex'), - ]); - const messageHash = getKeccak256Hash(messageData); + const messageHash = getMessageHash('ethereum', 'messageId', '0x4976da71bF84D750b5451B053051158EC0A4E876', deployer); // Mock message approved await contract.setAccount({ @@ -1169,7 +1148,7 @@ describe('View functions', () => { ...baseKvs(), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); @@ -1208,7 +1187,7 @@ describe('View functions', () => { ...baseKvs(), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str('1')), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); @@ -1432,7 +1411,7 @@ describe('View functions', () => { ], ), ], - }).assertFail({ code: 4, message: 'Invalid signers' }) + }).assertFail({ code: 4, message: 'Invalid signers' }); }); }); @@ -1453,16 +1432,9 @@ test('Approve validate execute external contract', async () => { pingPong, e.TopBuffer(payloadHash), ); - const messageData = Buffer.concat([ - Buffer.from(OTHER_CHAIN_NAME), - Buffer.from(MESSAGE_ID), - Buffer.from(OTHER_CHAIN_ADDRESS), - pingPong.toTopU8A(), - Buffer.from(payloadHash, 'hex'), - ]); - const messageHash = getKeccak256Hash(messageData); + const messageHash = getMessageHash(OTHER_CHAIN_NAME, MESSAGE_ID, OTHER_CHAIN_ADDRESS, pingPong, payloadHash); - const commandId = getKeccak256Hash(OTHER_CHAIN_NAME + '_' + MESSAGE_ID); + const crossChainId = e.Tuple(e.Str(OTHER_CHAIN_NAME), e.Str(MESSAGE_ID)); await deployer.callContract({ callee: contract, @@ -1486,7 +1458,7 @@ test('Approve validate execute external contract', async () => { ...baseKvs(), // Message was approved - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); @@ -1544,7 +1516,7 @@ test('Approve validate execute external contract', async () => { ...baseKvs(), // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str('1')), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); // Other user received funds @@ -1637,7 +1609,10 @@ test.skip('Approve messages with multisig prover encoded data', async () => { ], }); - const commandId = getKeccak256Hash('ganache-1_0xff822c88807859ff226b58e24f24974a70f04b9442501ae38fd665b3c68f3834-0'); + const crossChainId = e.Tuple( + e.Str('ganache-1'), + e.Str('0xff822c88807859ff226b58e24f24974a70f04b9442501ae38fd665b3c68f3834-0'), + ); assertAccount(await contract.getAccountWithKvs(), { balance: 0, @@ -1647,8 +1622,8 @@ test.skip('Approve messages with multisig prover encoded data', async () => { e.kvs.Mapper('signer_hash_by_epoch', e.U(1)).Value(e.TopBuffer(multisigSignersHash)), e.kvs.Mapper('epoch_by_signer_hash', e.TopBuffer(multisigSignersHash)).Value(e.U(1)), - // Message was executed - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer( + // Message was approved + e.kvs.Mapper('messages', crossChainId).Value(e.TopBuffer( 'b8fe5d23fe5954fa900ec37278b8661a98d061dfa34e7ebdd2e3ae98fbee5d8d')), ], }); diff --git a/tests/governance.test.ts b/tests/governance.test.ts index 391f16e..32ae690 100644 --- a/tests/governance.test.ts +++ b/tests/governance.test.ts @@ -1,6 +1,6 @@ import { afterEach, assert, beforeEach, test } from 'vitest'; import { assertAccount, d, e, Encodable, SContract, SWallet, SWorld } from 'xsuite'; -import { ADDRESS_ZERO, getKeccak256Hash, MESSAGE_ID } from './helpers'; +import { ADDRESS_ZERO, getKeccak256Hash, getMessageHash, MESSAGE_ID } from './helpers'; import createKeccakHash from 'keccak'; import fs from 'fs'; import { baseGatewayKvs, deployGatewayContract, gateway } from './itsHelpers'; @@ -71,16 +71,9 @@ const deployContract = async () => { const mockCallApprovedByGateway = async (payload: Encodable) => { const payloadHash = getKeccak256Hash(Buffer.from(payload.toTopU8A())); - const messageData = Buffer.concat([ - Buffer.from(GOVERNANCE_CHAIN), - Buffer.from(MESSAGE_ID), - Buffer.from(GOVERNANCE_ADDRESS), - contract.toTopU8A(), - Buffer.from(payloadHash, 'hex'), - ]); - const messageHash = getKeccak256Hash(messageData); + const messageHash = getMessageHash(GOVERNANCE_CHAIN, MESSAGE_ID, GOVERNANCE_ADDRESS, contract, payloadHash); - const commandId = getKeccak256Hash(GOVERNANCE_CHAIN + '_' + MESSAGE_ID); + const crossChainId = e.Tuple(e.Str(GOVERNANCE_CHAIN), e.Str(MESSAGE_ID)); // Mock call approved by gateway await gateway.setAccount({ @@ -90,7 +83,7 @@ const mockCallApprovedByGateway = async (payload: Encodable) => { ...baseGatewayKvs(deployer), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); } diff --git a/tests/helpers.ts b/tests/helpers.ts index 922c03e..d5a173c 100644 --- a/tests/helpers.ts +++ b/tests/helpers.ts @@ -2,6 +2,7 @@ import fs from 'fs'; import { UserSecretKey } from '@multiversx/sdk-wallet/out'; import createKeccakHash from 'keccak'; import { e, Encodable } from 'xsuite'; +import { Buffer } from 'buffer'; export const MOCK_CONTRACT_ADDRESS_1: string = 'erd1qqqqqqqqqqqqqpgqd77fnev2sthnczp2lnfx0y5jdycynjfhzzgq6p3rax'; export const MOCK_CONTRACT_ADDRESS_2: string = 'erd1qqqqqqqqqqqqqpgq7ykazrzd905zvnlr88dpfw06677lxe9w0n4suz00uh'; @@ -79,6 +80,23 @@ export const generateRotateSignersSignature = (signersHash: Buffer, data: Encoda return privateKey.sign(Buffer.from(messageHashToSign, 'hex')); }; +export const getMessageHash = ( + sourceChain: string, + messageId: string, + sourceAddress: string, + contractAddress: Encodable, + payloadHash: string = PAYLOAD_HASH +): Encodable => { + const messageData = Buffer.concat([ + e.Tuple(e.Str(sourceChain), e.Str(messageId)).toNestU8A(), + e.Str(sourceAddress).toNestU8A(), + contractAddress.toTopU8A(), + Buffer.from(payloadHash, 'hex'), + ]); + + return e.TopBuffer(getKeccak256Hash(messageData)); +}; + export const getSignersHash = (signers: { signer: string, weight: number } [], threshold: number, nonce: string) => { let signersLengthHex = numberToHex(signers.length, 4); @@ -115,7 +133,7 @@ const numberToHex = (nb: number, size: number = 0): string => { } return nbHex; -} +}; export const getSignersHashAndEncodable = (signers: { signer: string, @@ -140,6 +158,6 @@ export const generateProof = (weightedSigners: Encodable, signatures: (Buffer | }))); }; -export const getKeccak256Hash = (payload: string | Buffer = 'commandId') => { +export const getKeccak256Hash = (payload: string | Buffer) => { return createKeccakHash('keccak256').update(Buffer.from(payload)).digest('hex'); }; diff --git a/tests/its/itsExecuteDeployInterchainToken.test.ts b/tests/its/itsExecuteDeployInterchainToken.test.ts index 31bec25..9271a4d 100644 --- a/tests/its/itsExecuteDeployInterchainToken.test.ts +++ b/tests/its/itsExecuteDeployInterchainToken.test.ts @@ -13,8 +13,8 @@ import { Buffer } from 'buffer'; import { baseGatewayKvs, baseItsKvs, - deployContracts, deployTokenManagerInterchainToken, - deployTokenManagerMintBurn, + deployContracts, + deployTokenManagerInterchainToken, gateway, interchainTokenFactory, its, @@ -89,13 +89,13 @@ const mockGatewayCall = async (tokenId = INTERCHAIN_TOKEN_ID) => { ], ).substring(2); - const { commandId, messageHash } = await mockGatewayMessageApproved(payload, deployer); + const { crossChainId, messageHash } = await mockGatewayMessageApproved(payload, deployer); - return { payload, commandId, messageHash }; + return { payload, crossChainId, messageHash }; }; test('Only deploy token manager', async () => { - const { payload, commandId, messageHash } = await mockGatewayCall(); + const { payload, crossChainId, messageHash } = await mockGatewayCall(); await user.callContract({ callee: its, @@ -134,7 +134,7 @@ test('Only deploy token manager', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); }); @@ -152,7 +152,7 @@ test('Only issue esdt', async () => { ], }); - const { payload, commandId } = await mockGatewayCall(); + const { payload, crossChainId } = await mockGatewayCall(); await user.callContract({ callee: its, @@ -200,7 +200,7 @@ test('Only issue esdt', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); }); diff --git a/tests/its/itsExecuteDeployTokenManager.test.ts b/tests/its/itsExecuteDeployTokenManager.test.ts index d22f091..e1fb313 100644 --- a/tests/its/itsExecuteDeployTokenManager.test.ts +++ b/tests/its/itsExecuteDeployTokenManager.test.ts @@ -92,13 +92,13 @@ const mockGatewayCall = async (tokenId = INTERCHAIN_TOKEN_ID, type = TOKEN_MANAG ], ).substring(2); - const { commandId, messageHash } = await mockGatewayMessageApproved(payload, deployer); + const { crossChainId, messageHash } = await mockGatewayMessageApproved(payload, deployer); - return { payload, commandId, messageHash }; + return { payload, crossChainId, messageHash }; }; test('Execute', async () => { - const { payload, commandId} = await mockGatewayCall(); + const { payload, crossChainId } = await mockGatewayCall(); await user.callContract({ callee: its, @@ -138,7 +138,7 @@ test('Execute', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str('1')), ], }); }); @@ -176,7 +176,7 @@ test('Errors', async () => { ], }).assertFail({ code: 4, message: 'Not approved by gateway' }); - const { computedTokenId, } = await itsDeployTokenManagerLockUnlock( + const { computedTokenId } = await itsDeployTokenManagerLockUnlock( world, user, ); diff --git a/tests/its/itsExecuteInterchainTransfer.test.ts b/tests/its/itsExecuteInterchainTransfer.test.ts index ce86951..503659b 100644 --- a/tests/its/itsExecuteInterchainTransfer.test.ts +++ b/tests/its/itsExecuteInterchainTransfer.test.ts @@ -95,15 +95,15 @@ const mockGatewayCall = async (interchainTokenId: string, payload: string | null ).substring(2); } - const { commandId, messageHash } = await mockGatewayMessageApproved(payload, deployer); + const { crossChainId, messageHash } = await mockGatewayMessageApproved(payload, deployer); - return { payload, commandId, messageHash }; + return { payload, crossChainId, messageHash }; }; test('Transfer mint burn', async () => { const { computedTokenId, tokenManager, baseTokenManagerKvs } = await itsDeployTokenManagerMintBurn(world, user); - const { payload, commandId } = await mockGatewayCall(computedTokenId); + const { payload, crossChainId } = await mockGatewayCall(computedTokenId); await user.callContract({ callee: its, @@ -138,7 +138,7 @@ test('Transfer mint burn', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); }); @@ -150,7 +150,7 @@ test('Transfer lock unlock', async () => { true, ); - const { payload, commandId } = await mockGatewayCall(computedTokenId); + const { payload, crossChainId } = await mockGatewayCall(computedTokenId); await user.callContract({ callee: its, @@ -189,7 +189,7 @@ test('Transfer lock unlock', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); }); @@ -281,7 +281,7 @@ test('Express executor', async () => { user, ); - const { payload, commandId } = await mockGatewayCall(computedTokenId); + const { payload, crossChainId } = await mockGatewayCall(computedTokenId); const expressExecuteHash = computeExpressExecuteHash(payload); @@ -330,7 +330,7 @@ test('Express executor', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); diff --git a/tests/its/itsExecuteInterchainTransferWithData.test.ts b/tests/its/itsExecuteInterchainTransferWithData.test.ts index 7b6d617..4edf623 100644 --- a/tests/its/itsExecuteInterchainTransferWithData.test.ts +++ b/tests/its/itsExecuteInterchainTransferWithData.test.ts @@ -87,9 +87,9 @@ const mockGatewayCall = async (tokenId: string, fnc = 'ping') => { ], ).substring(2); - const { commandId, messageHash } = await mockGatewayMessageApproved(payload, deployer); + const { crossChainId, messageHash } = await mockGatewayMessageApproved(payload, deployer); - return { payload, commandId, messageHash }; + return { payload, crossChainId, messageHash }; }; test('Transfer with data', async () => { @@ -102,7 +102,7 @@ test('Transfer with data', async () => { 'EGLD', ); - const { payload, commandId } = await mockGatewayCall(computedTokenId); + const { payload, crossChainId } = await mockGatewayCall(computedTokenId); await user.callContract({ callee: its, @@ -159,7 +159,7 @@ test('Transfer with data', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); }); @@ -174,7 +174,7 @@ test('Transfer with data contract error', async () => { 'EGLD', ); - const { payload, commandId } = await mockGatewayCall(computedTokenId, 'wrong'); + const { payload, crossChainId } = await mockGatewayCall(computedTokenId, 'wrong'); await user.callContract({ callee: its, @@ -219,7 +219,7 @@ test('Transfer with data contract error', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); }); @@ -234,7 +234,7 @@ test('Express executor', async () => { 'EGLD', ); - let { payload, commandId } = await mockGatewayCall(computedTokenId); + let { payload, crossChainId } = await mockGatewayCall(computedTokenId); const expressExecuteHash = computeExpressExecuteHash(payload); @@ -283,7 +283,7 @@ test('Express executor', async () => { kvs: [ ...baseGatewayKvs(deployer), - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); diff --git a/tests/its/itsExpressExecute.test.ts b/tests/its/itsExpressExecute.test.ts index 6887bc0..25eb5c3 100644 --- a/tests/its/itsExpressExecute.test.ts +++ b/tests/its/itsExpressExecute.test.ts @@ -405,7 +405,7 @@ test('Express execute errors', async () => { ], }).assertFail({ code: 4, message: 'Express executor already set' }); - const commandId = getKeccak256Hash(OTHER_CHAIN_NAME + '_' + MESSAGE_ID); + const crossChainId = e.Tuple(e.Str(OTHER_CHAIN_NAME), e.Str(MESSAGE_ID)); // Mock Gateway message already executed await gateway.setAccount({ @@ -415,7 +415,7 @@ test('Express execute errors', async () => { ...baseGatewayKvs(deployer), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.Str("1")), + e.kvs.Mapper('messages', crossChainId).Value(e.Str("1")), ], }); diff --git a/tests/itsHelpers.ts b/tests/itsHelpers.ts index 9c0fe1d..7abd4d7 100644 --- a/tests/itsHelpers.ts +++ b/tests/itsHelpers.ts @@ -6,7 +6,7 @@ import { CHAIN_NAME, CHAIN_NAME_HASH, DOMAIN_SEPARATOR, - getKeccak256Hash, + getKeccak256Hash, getMessageHash, getSignersHash, INTERCHAIN_TOKEN_ID, MESSAGE_ID, @@ -530,16 +530,9 @@ export const baseItsKvs = (operator: SWallet | SContract, interchainTokenFactory export async function mockGatewayMessageApproved(payload: string, operator: SWallet) { const payloadHash = getKeccak256Hash(Buffer.from(payload, 'hex')); - const messageData = Buffer.concat([ - Buffer.from(OTHER_CHAIN_NAME), - Buffer.from(MESSAGE_ID), - Buffer.from(OTHER_CHAIN_ADDRESS), - its.toTopU8A(), - Buffer.from(payloadHash, 'hex'), - ]); - const messageHash = getKeccak256Hash(messageData); + const messageHash = getMessageHash(OTHER_CHAIN_NAME, MESSAGE_ID, OTHER_CHAIN_ADDRESS, its, payloadHash); - const commandId = getKeccak256Hash(OTHER_CHAIN_NAME + '_' + MESSAGE_ID); + const crossChainId = e.Tuple(e.Str(OTHER_CHAIN_NAME), e.Str(MESSAGE_ID)); // Mock call approved by gateway await gateway.setAccount({ @@ -549,9 +542,9 @@ export async function mockGatewayMessageApproved(payload: string, operator: SWal ...baseGatewayKvs(operator), // Manually approve message - e.kvs.Mapper('messages', e.TopBuffer(commandId)).Value(e.TopBuffer(messageHash)), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); - return { commandId, messageHash }; + return { crossChainId, messageHash }; } From 54cbe2e40c948ff8dec61a58911276be5c42054a Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Tue, 19 Nov 2024 15:49:48 +0200 Subject: [PATCH 3/8] Audit Gas Service C1 & C2: Fix collect fees and set gas collector & fix clippy. --- gas-service/src/lib.rs | 14 +++---- gateway/src/auth.rs | 2 +- gateway/src/lib.rs | 4 +- interchain-token-service/src/proxy_gmp.rs | 2 +- tests/gmp/gasService.test.ts | 47 +++++++++++++++++++++-- 5 files changed, 53 insertions(+), 16 deletions(-) diff --git a/gas-service/src/lib.rs b/gas-service/src/lib.rs index b9bd301..d03dbe7 100644 --- a/gas-service/src/lib.rs +++ b/gas-service/src/lib.rs @@ -236,8 +236,6 @@ pub trait GasService: events::Events { let tokens_vec = tokens.into_vec(); let amounts_vec = amounts.into_vec(); - let mut payments = ManagedVec::new(); - for index in 0..tokens_length { let token: EgldOrEsdtTokenIdentifier = tokens_vec.get(index); let amount = amounts_vec.get(index).clone_value(); @@ -251,13 +249,9 @@ pub trait GasService: events::Events { self.send().direct_egld(receiver, &amount); } } else if amount <= balance { - payments.push(EsdtTokenPayment::new(token.unwrap_esdt(), 0, amount)); + self.send().direct_esdt(receiver, &token.unwrap_esdt(), 0, &amount) } } - - if !payments.is_empty() { - self.send().direct_multi(receiver, &payments); - } } #[endpoint(refund)] @@ -288,7 +282,11 @@ pub trait GasService: events::Events { #[endpoint(setGasCollector)] fn set_gas_collector(&self, gas_collector: &ManagedAddress) { - self.require_only_collector(); + let caller = self.blockchain().get_caller(); + let collector = self.gas_collector().get(); + let owner = self.blockchain().get_owner_address(); + + require!(caller == collector || caller == owner, "Not collector or owner"); self.gas_collector().set(gas_collector); } diff --git a/gateway/src/auth.rs b/gateway/src/auth.rs index f471621..afbd0c5 100644 --- a/gateway/src/auth.rs +++ b/gateway/src/auth.rs @@ -169,7 +169,7 @@ pub trait AuthModule: events::Events { ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); - let _ = signers + signers .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode weighted signers")); diff --git a/gateway/src/lib.rs b/gateway/src/lib.rs index 5f427e0..386f249 100644 --- a/gateway/src/lib.rs +++ b/gateway/src/lib.rs @@ -177,11 +177,11 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); - let _ = cross_chain_id + cross_chain_id .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode cross chain id")); - let _ = source_address + source_address .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode source address")); diff --git a/interchain-token-service/src/proxy_gmp.rs b/interchain-token-service/src/proxy_gmp.rs index fdb5611..684ccf1 100644 --- a/interchain-token-service/src/proxy_gmp.rs +++ b/interchain-token-service/src/proxy_gmp.rs @@ -153,7 +153,7 @@ pub trait ProxyGmpModule: address_tracker::AddressTracker { source_chain, message_id, source_address, - &self.blockchain().get_sc_address(), + self.blockchain().get_sc_address(), payload_hash, ) .execute_on_dest_context::() diff --git a/tests/gmp/gasService.test.ts b/tests/gmp/gasService.test.ts index 3b9674c..0d3fad9 100644 --- a/tests/gmp/gasService.test.ts +++ b/tests/gmp/gasService.test.ts @@ -633,6 +633,25 @@ test('Collect fees too much asked', async () => { ], }); + await collector.callContract({ + callee: contract, + gasLimit: 10_000_000, + funcName: 'collectFees', + funcArgs: [ + e.Addr(deployer.toString()), + + e.U32(3), + e.Str(TOKEN_ID), + e.Str(TOKEN_ID), + e.Str('EGLD'), + + e.U32(3), + e.U(750), + e.U(750), // Higher than remaining balance, will be ignored + e.U(20_000), + ], + }); + await collector.callContract({ callee: contract, gasLimit: 10_000_000, @@ -659,7 +678,7 @@ test('Collect fees too much asked', async () => { e.kvs.Esdts([ { id: TOKEN_ID, - amount: 1_000, + amount: 250, }, ]), ], @@ -799,19 +818,39 @@ test('Refund esdt', async () => { test('Set gas collector not collector', async () => { await deployContract(); - await deployer.callContract({ + let user = await world.createWallet(); + + await user.callContract({ callee: contract, gasLimit: 10_000_000, funcName: 'setGasCollector', funcArgs: [ deployer, ], - }).assertFail({ code: 4, message: 'Not collector' }); + }).assertFail({ code: 4, message: 'Not collector or owner' }); }); test('Set gas collector', async () => { await deployContract(); + // Deployer can also set collector + await deployer.callContract({ + callee: contract, + gasLimit: 10_000_000, + funcName: 'setGasCollector', + funcArgs: [ + collector, + ], + }); + + let pairs = await contract.getAccountWithKvs(); + assertAccount(pairs, { + balance: 0n, + kvs: [ + e.kvs.Mapper('gas_collector').Value(collector), + ], + }); + await collector.callContract({ callee: contract, gasLimit: 10_000_000, @@ -821,7 +860,7 @@ test('Set gas collector', async () => { ], }); - const pairs = await contract.getAccountWithKvs(); + pairs = await contract.getAccountWithKvs(); assertAccount(pairs, { balance: 0n, kvs: [ From 07618f91091aee2e2c01914c4be94f59a142fc36 Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Tue, 19 Nov 2024 17:00:15 +0200 Subject: [PATCH 4/8] Audit Governance C1 & C2 & C3: Add min gas for proposal, accept esdt tokens and return tokens if async call fail. Use nest encode for proposal hash for consistency. --- governance/src/lib.rs | 77 +++++++++++-- tests/governance.test.ts | 234 ++++++++++++++++++++++++++++++--------- 2 files changed, 245 insertions(+), 66 deletions(-) diff --git a/governance/src/lib.rs b/governance/src/lib.rs index bf3078e..9e7fdca 100644 --- a/governance/src/lib.rs +++ b/governance/src/lib.rs @@ -19,6 +19,7 @@ pub enum GovernanceCommand { pub struct DecodedCallData { pub endpoint_name: ManagedBuffer, pub arguments: ManagedVec>, + pub min_gas_limit: u64, } #[derive(TypeAbi, TopDecode)] @@ -30,7 +31,8 @@ pub struct ExecutePayload { pub eta: u64, } -const EXECUTE_PROPOSAL_CALLBACK_GAS: u64 = 5_000_000; +const EXECUTE_PROPOSAL_CALLBACK_GAS: u64 = 10_000_000; +const EXECUTE_PROPOSAL_CALLBACK_GAS_PER_PAYMENT: u64 = 2_000_000; // This is overkill, but the callback should be prevented from failing at all costs const KEEP_EXTRA_GAS: u64 = 15_000_000; // Extra gas to keep in contract before registering async promise. This needs to be a somewhat larger value @@ -63,7 +65,7 @@ pub trait Governance: events::Events { #[upgrade] fn upgrade(&self) {} - #[payable("EGLD")] + #[payable("*")] #[endpoint(executeProposal)] fn execute_proposal( &self, @@ -95,19 +97,55 @@ pub trait Governance: events::Events { "Not enough gas left for async call" ); - let gas_limit = gas_left - EXECUTE_PROPOSAL_CALLBACK_GAS - KEEP_EXTRA_GAS; + let mut gas_limit = gas_left - EXECUTE_PROPOSAL_CALLBACK_GAS - KEEP_EXTRA_GAS; + + require!( + gas_limit >= decoded_call_data.min_gas_limit, + "Insufficient gas for execution" + ); + + let caller = self.blockchain().get_caller(); + + let mut extra_gas_for_callback = EXECUTE_PROPOSAL_CALLBACK_GAS; + + let payments = self.call_value().any_payment(); + + match payments.as_refs() { + EgldOrMultiEsdtPaymentRefs::Egld(egld_value) => { + if native_value > 0 { + require!(egld_value >= &native_value, "Not enough egld sent"); + } + } + EgldOrMultiEsdtPaymentRefs::MultiEsdt(payments) => { + if native_value > 0 { + require!(payments.is_empty(), "No egld payment sent"); + } + + let gas_for_payments = EXECUTE_PROPOSAL_CALLBACK_GAS_PER_PAYMENT * payments.len() as u64; + + require!( + gas_limit >= gas_for_payments, + "Not enough gas left for async call payments" + ); + + gas_limit -= gas_for_payments; + extra_gas_for_callback += gas_for_payments; + } + } self.send() .contract_call::<()>(target, decoded_call_data.endpoint_name) - .with_egld_transfer(native_value) + .with_any_payment(payments.clone()) .with_raw_arguments(decoded_call_data.arguments.into()) .with_gas_limit(gas_limit) .async_call_promise() - .with_callback( - self.callbacks() - .execute_proposal_callback(&proposal_hash, eta), - ) - .with_extra_gas_for_callback(EXECUTE_PROPOSAL_CALLBACK_GAS) + .with_callback(self.callbacks().execute_proposal_callback( + &proposal_hash, + eta, + caller, + payments, + )) + .with_extra_gas_for_callback(extra_gas_for_callback) .register_promise(); } @@ -240,8 +278,14 @@ pub trait Governance: events::Events { let mut encoded = ManagedBuffer::new(); encoded.append(target.as_managed_buffer()); - encoded.append(call_data); - encoded.append(&native_value.to_bytes_be_buffer()); + + call_data + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode call data")); + + native_value + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode native value")); self.crypto().keccak256(encoded) } @@ -259,6 +303,8 @@ pub trait Governance: events::Events { &self, hash: &ManagedByteArray, eta: u64, + caller: ManagedAddress, + payments: EgldOrMultiEsdtPayment, #[call_result] call_result: ManagedAsyncCallResult>, ) { match call_result { @@ -266,6 +312,15 @@ pub trait Governance: events::Events { self.execute_proposal_success_event(hash, results); } ManagedAsyncCallResult::Err(err) => { + match payments { + EgldOrMultiEsdtPayment::Egld(egld_value) => { + self.send().direct_non_zero_egld(&caller, &egld_value); + } + EgldOrMultiEsdtPayment::MultiEsdt(esdts) => { + self.send().direct_multi(&caller, &esdts); + } + } + // Let call be retried in case of failure, mostly because async call // can fail with out of gas since it can be triggered by anyone self.time_lock_eta(hash).set(eta); diff --git a/tests/governance.test.ts b/tests/governance.test.ts index 32ae690..f72ff3e 100644 --- a/tests/governance.test.ts +++ b/tests/governance.test.ts @@ -1,9 +1,10 @@ import { afterEach, assert, beforeEach, test } from 'vitest'; import { assertAccount, d, e, Encodable, SContract, SWallet, SWorld } from 'xsuite'; -import { ADDRESS_ZERO, getKeccak256Hash, getMessageHash, MESSAGE_ID } from './helpers'; +import { ADDRESS_ZERO, getKeccak256Hash, getMessageHash, MESSAGE_ID, PAYLOAD_HASH, TOKEN_ID } from './helpers'; import createKeccakHash from 'keccak'; import fs from 'fs'; import { baseGatewayKvs, deployGatewayContract, gateway } from './itsHelpers'; +import { Buffer } from 'buffer'; const GOVERNANCE_CHAIN = 'Axelar'; const GOVERNANCE_ADDRESS = 'axelar1u5jhn5876mjzmgw7j37mdvqh4qp5y6z2gc6rc3'; @@ -22,6 +23,9 @@ beforeEach(async () => { deployer = await world.createWallet({ balance: 10_000_000_000n, + kvs: [ + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), + ], }); }); @@ -86,7 +90,21 @@ const mockCallApprovedByGateway = async (payload: Encodable) => { e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); -} +}; + +const getProposalHash = ( + target: Encodable, + callDataTopBuffer: Encodable, + nativeValue: Encodable, +): Encodable => { + const hashData = Buffer.concat([ + target.toTopU8A(), + e.Buffer(callDataTopBuffer.toTopU8A()).toNestU8A(), + nativeValue.toNestU8A(), + ]); + + return e.TopBuffer(getKeccak256Hash(hashData)); +}; test('Init errors', async () => { await deployGatewayContract(deployer); @@ -146,12 +164,7 @@ test('Execute proposal errors', async () => { ], }).assertFail({ code: 4, message: 'Invalid time lock hash' }); - const buffer = Buffer.concat([ - gateway.toTopU8A(), - wrongCallData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(gateway, wrongCallData, e.U(0)); // Mock hash await contract.setAccount({ @@ -159,7 +172,7 @@ test('Execute proposal errors', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(1)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), ], }); @@ -189,6 +202,55 @@ test('Execute proposal errors', async () => { }).assertFail({ code: 4, message: 'Could not decode call data' }); }); +test('Execute proposal esdt errors', async () => { + await deployContract(); + + // Increase timestamp so finalize_time_lock passes + await world.setCurrentBlockInfo({ timestamp: 1 }); + + const callData = e.TopBuffer(e.Tuple( + e.Str('function'), + e.List(), + e.U64(0), // min gas limit + ).toTopU8A()); + + const proposalHashWithEgld = getProposalHash(gateway, callData, e.U(1_000)); + + // Mock hash + await contract.setAccount({ + ...await contract.getAccountWithKvs(), + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('time_lock_eta', proposalHashWithEgld).Value(e.U64(1)), + ], + }); + + await deployer.callContract({ + callee: contract, + gasLimit: 100_000_000, + funcName: 'executeProposal', + value: 999, + funcArgs: [ + gateway, + callData, + e.U(1_000), + ], + }).assertFail({ code: 4, message: 'Not enough egld sent' }); + + await deployer.callContract({ + callee: contract, + gasLimit: 100_000_000, + funcName: 'executeProposal', + funcArgs: [ + gateway, + callData, + e.U(1_000), + ], + esdts: [{ id: TOKEN_ID, amount: 1_000 }], + }).assertFail({ code: 4, message: 'No egld payment sent' }); +}); + test('Execute proposal upgrade gateway', async () => { await deployContract(); @@ -203,14 +265,10 @@ test('Execute proposal upgrade gateway', async () => { e.Buffer('0100'), // upgrade metadata (upgradable) e.Buffer(newOperator.toTopU8A()), // Arguments to upgrade function fo Gateway ), + e.U64(20_000_000), // min gas limit ).toTopU8A()); - const buffer = Buffer.concat([ - gateway.toTopU8A(), - callData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(gateway, callData, e.U(0)); // Mock hash await contract.setAccount({ @@ -218,7 +276,7 @@ test('Execute proposal upgrade gateway', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(1)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), ], }); // Increase timestamp so finalize_time_lock passes @@ -226,7 +284,7 @@ test('Execute proposal upgrade gateway', async () => { await deployer.callContract({ callee: contract, - gasLimit: 20_000_000, + gasLimit: 30_000_000, funcName: 'executeProposal', funcArgs: [ gateway, @@ -235,6 +293,17 @@ test('Execute proposal upgrade gateway', async () => { ], }).assertFail({ code: 4, message: 'Not enough gas left for async call' }); + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'executeProposal', + funcArgs: [ + gateway, + callData, + e.U(0), + ], + }).assertFail({ code: 4, message: 'Insufficient gas for execution' }); + await deployer.callContract({ callee: contract, gasLimit: 200_000_000, @@ -270,14 +339,10 @@ test('Execute proposal upgrade gateway error', async () => { e.Buffer('0100'), // upgrade metadata (upgradable) e.Str('wrongArgs'), ), + e.U64(1_000_000), // min gas limit ).toTopU8A()); - const buffer = Buffer.concat([ - gateway.toTopU8A(), - callData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(gateway, callData, e.U(1_000)); // Mock hash await contract.setAccount({ @@ -285,7 +350,7 @@ test('Execute proposal upgrade gateway error', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(1)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), ], }); // Increase timestamp so finalize_time_lock passes @@ -295,11 +360,82 @@ test('Execute proposal upgrade gateway error', async () => { callee: contract, gasLimit: 50_000_000, funcName: 'executeProposal', + value: 1_000, // also send egld + funcArgs: [ + gateway, + callData, + e.U(1_000), + ], + }); // async call actually fails + + // Time lock eta was NOT deleted + let kvs = await contract.getAccountWithKvs(); + assertAccount(kvs, { + balance: 0n, + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + ], + }); + + assertAccount(await deployer.getAccountWithKvs(), { + balance: 10_000_000_000n, // got egld back + }); +}); + + +test('Execute proposal upgrade gateway esdt error', async () => { + await deployContract(); + + const gatewayCode = fs.readFileSync('gateway/output/gateway.wasm'); + + const callData = e.TopBuffer(e.Tuple( + e.Str('upgradeContract'), + e.List( + e.Buffer(gatewayCode), // code + e.Buffer('0100'), // upgrade metadata (upgradable) + e.Str('wrongArgs'), + ), + e.U64(1_000_000), // min gas limit + ).toTopU8A()); + + const proposalHash = getProposalHash(gateway, callData, e.U(0)); + + // Mock hash + await contract.setAccount({ + ...await contract.getAccountWithKvs(), + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + ], + }); + // Increase timestamp so finalize_time_lock passes + await world.setCurrentBlockInfo({ timestamp: 1 }); + + await deployer.callContract({ + callee: contract, + gasLimit: 47_000_000, + funcName: 'executeProposal', funcArgs: [ gateway, callData, e.U(0), ], + esdts: [{ id: TOKEN_ID, amount: 1_000 }], + }).assertFail({ code: 4, message: 'Not enough gas left for async call payments' }); + + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'executeProposal', + funcArgs: [ + gateway, + callData, + e.U(0), + ], + esdts: [{ id: TOKEN_ID, amount: 1_000 }], }); // async call actually fails // Time lock eta was NOT deleted @@ -309,7 +445,14 @@ test('Execute proposal upgrade gateway error', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(1)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + ], + }); + + assertAccount(await deployer.getAccountWithKvs(), { + balance: 10_000_000_000n, + kvs: [ + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), // got esdt back ], }); }); @@ -334,14 +477,10 @@ test('Withdraw', async () => { e.Buffer(deployer.toNestBytes()), e.U(100), ), + e.U64(1_000_000), // min gas limit ).toTopU8A()); - const buffer = Buffer.concat([ - contract.toTopU8A(), - callData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(contract, callData, e.U(0)); // Mock hash & balance await contract.setAccount({ @@ -350,7 +489,7 @@ test('Withdraw', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(1)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), ], }); // Increase timestamp so finalize_time_lock passes @@ -483,12 +622,7 @@ test('Execute schedule time lock proposal min eta', async () => { ], }); - const buffer = Buffer.concat([ - gateway.toTopU8A(), - callData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(gateway, callData, e.U(0)); let kvs = await contract.getAccountWithKvs(); assertAccount(kvs, { @@ -496,7 +630,7 @@ test('Execute schedule time lock proposal min eta', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(10)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(10)), ], }); }); @@ -526,12 +660,7 @@ test('Execute schedule time lock proposal eta', async () => { ], }); - const buffer = Buffer.concat([ - gateway.toTopU8A(), - callData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(gateway, callData, e.U(0)); let kvs = await contract.getAccountWithKvs(); assertAccount(kvs, { @@ -539,7 +668,7 @@ test('Execute schedule time lock proposal eta', async () => { kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(11)), + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(11)), ], }); @@ -563,7 +692,7 @@ test('Execute schedule time lock proposal eta', async () => { gateway, callData, e.U(0), - ] + ], }); assert(d.U64().topDecode(result.returnData[0]) === 11n); }); @@ -582,20 +711,15 @@ test('Execute cancel time lock proposal', async () => { await mockCallApprovedByGateway(payload); // Mock time lock era set - const buffer = Buffer.concat([ - gateway.toTopU8A(), - callData.toTopU8A(), - e.U(0).toTopU8A(), - ]); - const hash = createKeccakHash('keccak256').update(buffer).digest('hex'); + const proposalHash = getProposalHash(gateway, callData, e.U(0)); await contract.setAccount({ ...await contract.getAccountWithKvs(), kvs: [ ...baseKvs(), - e.kvs.Mapper('time_lock_eta', e.TopBuffer(hash)).Value(e.U64(10)), - ] + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(10)), + ], }); await deployer.callContract({ From bfd81fdfcbc7e76701bd3aae0ee99ebb7d9f9003 Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Wed, 20 Nov 2024 13:58:49 +0200 Subject: [PATCH 5/8] Update tests after cosm wasm updates. --- .github/workflows/actions.yml | 13 ++--- gateway/src/auth.rs | 3 +- tests/gmp/gateway.test.ts | 96 +++++++++++++++++++++++++---------- tests/helpers.ts | 1 - 4 files changed, 78 insertions(+), 35 deletions(-) diff --git a/.github/workflows/actions.yml b/.github/workflows/actions.yml index 79e9cd8..fecd9af 100644 --- a/.github/workflows/actions.yml +++ b/.github/workflows/actions.yml @@ -14,10 +14,10 @@ permissions: jobs: contracts: name: Contracts - uses: multiversx/mx-sc-actions/.github/workflows/contracts.yml@v3.2.0 + uses: multiversx/mx-sc-actions/.github/workflows/contracts.yml@v3.3.1 with: - rust-toolchain: 1.78.0 - pip-mxpy-args: "multiversx-sdk-cli==v9.5.5" + rust-toolchain: 1.81.0 + pip-mxpy-args: "multiversx-sdk-cli==v9.7.0" secrets: token: ${{ secrets.GITHUB_TOKEN }} @@ -25,7 +25,7 @@ jobs: name: xSuite Tests runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v3 - name: Install rust uses: actions-rust-lang/setup-rust-toolchain@v1 @@ -35,7 +35,7 @@ jobs: - name: Install prerequisites run: | - pipx install multiversx-sdk-cli==v9.5.5 + pipx install "multiversx-sdk-cli==v9.7.0" wget -O binaryen.tar.gz https://github.com/WebAssembly/binaryen/releases/download/version_112/binaryen-version_112-x86_64-linux.tar.gz tar -xf binaryen.tar.gz @@ -43,9 +43,10 @@ jobs: sudo apt install -y wabt=1.0.27-1 + cargo install wasm-opt cargo install twiggy - cargo install multiversx-sc-meta --locked + cargo install multiversx-sc-meta --version 0.50.3 --locked which mxpy which wasm-opt diff --git a/gateway/src/auth.rs b/gateway/src/auth.rs index afbd0c5..b2bae7a 100644 --- a/gateway/src/auth.rs +++ b/gateway/src/auth.rs @@ -169,8 +169,9 @@ pub trait AuthModule: events::Events { ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); + // Will nest encode struct fields signers - .dep_encode(&mut encoded) + .top_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode weighted signers")); self.crypto().keccak256(encoded) diff --git a/tests/gmp/gateway.test.ts b/tests/gmp/gateway.test.ts index d5ff64e..059047b 100644 --- a/tests/gmp/gateway.test.ts +++ b/tests/gmp/gateway.test.ts @@ -19,7 +19,6 @@ import { TOKEN_ID2, } from '../helpers'; import { deployPingPongInterchain, pingPong } from '../itsHelpers'; -import { Buffer } from 'buffer'; let world: SWorld; let deployer: SWallet; @@ -79,6 +78,17 @@ const defaultWeightedSigners = e.Tuple( e.TopBuffer(getKeccak256Hash('nonce1')), ); +// Signers hash used in Ampd to verify that the computation there is correct. Uncomment and log to display +// const ampdSignersHash = getSignersHash( +// [ +// { signer: '45e67eaf446e6c26eb3a2b55b64339ecf3a4d1d03180bee20eb5afdd23fa644f', weight: 1 }, +// { signer: 'c387253d29085a8036d6ae2cafb1b14699751417c0ce302cfe03da279e6b5c04', weight: 1 }, +// { signer: 'dd9822c7fa239dda9913ebee813ecbe69e35d88ff651548d5cc42c033a8a667b', weight: 1 }, +// ], +// 2, +// '0000000000000000000000000000000000000000000000000000000000000005', +// ); + const defaultSignersHash = getSignersHash( [ { signer: ALICE_PUB_KEY, weight: 5 }, @@ -1543,6 +1553,7 @@ test('Approve validate execute external contract', async () => { }); }); +// Signers hash used in CosmWasm to verify that the computation there is correct. const multisigSignersHash = getSignersHash( [ { signer: ALICE_PUB_KEY, weight: 1 }, @@ -1555,8 +1566,27 @@ const multisigSignersHash = getSignersHash( '290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563', ); -// TODO: Update CosmWasm contracts -test.skip('Approve messages with multisig prover encoded data', async () => { +test('Approve messages with multisig prover encoded data', async () => { + const message = e.Tuple( + e.Str('ganache-1'), + e.Str('0xff822c88807859ff226b58e24f24974a70f04b9442501ae38fd665b3c68f3834-0'), + e.Str('0x52444f1835Adc02086c37Cb226561605e2E1699b'), + e.Addr('erd1qqqqqqqqqqqqqpgqd77fnev2sthnczp2lnfx0y5jdycynjfhzzgq6p3rax'), + e.TopBuffer('8c3685dc41c2eca11426f8035742fb97ea9f14931152670a5703f18fe8b392f0'), + ); + const messageHash = getMessageHash( + 'ganache-1', + '0xff822c88807859ff226b58e24f24974a70f04b9442501ae38fd665b3c68f3834-0', + '0x52444f1835Adc02086c37Cb226561605e2E1699b', + e.Addr('erd1qqqqqqqqqqqqqpgqd77fnev2sthnczp2lnfx0y5jdycynjfhzzgq6p3rax'), + '8c3685dc41c2eca11426f8035742fb97ea9f14931152670a5703f18fe8b392f0', + ); + + // We need real signatures here + const aliceSignature = generateMessageSignature(multisigSignersHash, e.List(message)).toString('hex'); + const bobSignature = generateMessageSignature(multisigSignersHash, e.List(message), './bob.pem').toString('hex'); + const carolSignature = generateMessageSignature(multisigSignersHash, e.List(message), './carol.pem').toString('hex'); + await deployContract(); // Mock multisig signers in contract @@ -1589,13 +1619,13 @@ test.skip('Approve messages with multisig prover encoded data', async () => { // 00000001 03 - length of biguint threshold followed by 3 as hex // 290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 - the nonce (keccak256 hash of Uin256 number 0, created_at date) // 00000005 - length of signatures - // 01 9543286a58ded1c031fcf8e5fcdc7c5b48b6304c539bdf7a30a0b780451a64318420fe654a13be7a33cae4f221cd26e1e033d01da144901453474c73b520450d - first signature encoded as a Some option - // 01 199b7e0f25ff4c24637bbdfdc18d338f422793f492a81140afd080019061088ddf667f018d88928a28dcb77a2c253c66ee5a83be2d4134ff3ab3141f0fdb170d - second signature encoded as a Some option - // 01 4f883c316682c6e000bf4c92536a138f78c6af265f4f13d7210110e40350bb4d99e049677db13e7c12f8a4e617a5cb9bf32f5142cd58f7146505078e2d675703 - third signature encoded as a Some option + // 01 ${aliceSignature} - first signature encoded as a Some option + // 01 ${bobSignature} - second signature encoded as a Some option + // 01 ${carolSignature} - third signature encoded as a Some option // 00 - fourth signature encoded as a None option (the fourth signer didn't specify any signature) // 00 - fifth signature encoded as a None option (the fifth signer didn't specify any signature) const proof = Buffer.from( - '000000050139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba0000000101ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b60000000101ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a444900000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e56300000005019543286a58ded1c031fcf8e5fcdc7c5b48b6304c539bdf7a30a0b780451a64318420fe654a13be7a33cae4f221cd26e1e033d01da144901453474c73b520450d01199b7e0f25ff4c24637bbdfdc18d338f422793f492a81140afd080019061088ddf667f018d88928a28dcb77a2c253c66ee5a83be2d4134ff3ab3141f0fdb170d014f883c316682c6e000bf4c92536a138f78c6af265f4f13d7210110e40350bb4d99e049677db13e7c12f8a4e617a5cb9bf32f5142cd58f7146505078e2d6757030000', + `000000050139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba0000000101ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b60000000101ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a444900000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e5630000000501${aliceSignature}01${bobSignature}01${carolSignature}0000`, 'hex', ); @@ -1623,14 +1653,36 @@ test.skip('Approve messages with multisig prover encoded data', async () => { e.kvs.Mapper('epoch_by_signer_hash', e.TopBuffer(multisigSignersHash)).Value(e.U(1)), // Message was approved - e.kvs.Mapper('messages', crossChainId).Value(e.TopBuffer( - 'b8fe5d23fe5954fa900ec37278b8661a98d061dfa34e7ebdd2e3ae98fbee5d8d')), + e.kvs.Mapper('messages', crossChainId).Value(messageHash), ], }); }); -// TODO: Update CosmWasm contracts -test.skip('Rotate signers with multisig prover encoded data', async () => { +test('Rotate signers with multisig prover encoded data', async () => { + const newWeightedSigners = e.Tuple( + e.List( + e.Tuple(e.TopBuffer(ALICE_PUB_KEY), e.U(1)), + e.Tuple(e.TopBuffer(BOB_PUB_KEY), e.U(1)), + e.Tuple(e.TopBuffer(CAROL_PUB_KEY), e.U(1)), + ), + e.U(3), + e.TopBuffer('290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563'), + ); + const newSignersHash = getSignersHash( + [ + { signer: ALICE_PUB_KEY, weight: 1 }, + { signer: BOB_PUB_KEY, weight: 1 }, + { signer: CAROL_PUB_KEY, weight: 1 }, + ], + 3, + '290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563', + ); + + // We need real signatures here + const aliceSignature = generateRotateSignersSignature(multisigSignersHash, newWeightedSigners).toString('hex'); + const bobSignature = generateRotateSignersSignature(multisigSignersHash, newWeightedSigners, './bob.pem').toString('hex'); + const carolSignature = generateRotateSignersSignature(multisigSignersHash, newWeightedSigners, './carol.pem').toString('hex'); + await deployContract(); // Mock multisig signers in contract @@ -1652,7 +1704,7 @@ test.skip('Rotate signers with multisig prover encoded data', async () => { // b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba - third new signer // 00000001 01 - length of biguint weight followed by 1 as hex // 00000001 03 - length of biguint threshold followed by 3 as hex - // 290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 - the nonce (keccak256 hash of Uin256 number 0, created_at date) + // 290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 - the nonce (mock created at number as uint256) const newSigners = Buffer.from( '000000030139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba00000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563', 'hex', @@ -1665,15 +1717,15 @@ test.skip('Rotate signers with multisig prover encoded data', async () => { // ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b6 00000001 01 - fourth signer with weight // ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a4449 00000001 01 - fifth signer with weight // 00000001 03 - length of biguint threshold followed by 3 as hex - // 290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 - the nonce (keccak256 hash of Uin256 number 0, created_at date) + // 290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 - the nonce (mock created at number as uint256) // 00000005 - length of signatures - // 01 5c98a98d1e47adecf83a10d4fdc542aae1cb13ab8e6d3f5e237ad75ccb6608631c0d3f8735e3f5f481e82f088fe5215d431ae8c6abf68b96797df4bbe610cd05 - first signature encoded as a Some option - // 01 ca0999eac93ee855ea88680b8094660635a06743e9acdb8d1987a9c48a60e9f794bd22a10748bb9c3c961ddc3068a96abfae00a9c38252a4b3ad99caeb060805 - second signature encoded as a Some option - // 01 deca8b224a38ad99ec4cb4f3d8e86778544c55ab0c4513ce8af834b81b3e934eef29727cc76c364f316a44c2eea82fa655f209f0c5205a209461d8a7fbbacf03 - third signature encoded as a Some option + // 01 ${aliceSignature} - first signature encoded as a Some option + // 01 ${bobSignature} - second signature encoded as a Some option + // 01 ${carolSignature} - third signature encoded as a Some option // 00 - fourth signature encoded as a None option (the fourth signer didn't specify any signature) // 00 - fifth signature encoded as a None option (the fifth signer didn't specify any signature) const proof = Buffer.from( - '000000050139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba0000000101ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b60000000101ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a444900000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e56300000005015c98a98d1e47adecf83a10d4fdc542aae1cb13ab8e6d3f5e237ad75ccb6608631c0d3f8735e3f5f481e82f088fe5215d431ae8c6abf68b96797df4bbe610cd0501ca0999eac93ee855ea88680b8094660635a06743e9acdb8d1987a9c48a60e9f794bd22a10748bb9c3c961ddc3068a96abfae00a9c38252a4b3ad99caeb06080501deca8b224a38ad99ec4cb4f3d8e86778544c55ab0c4513ce8af834b81b3e934eef29727cc76c364f316a44c2eea82fa655f209f0c5205a209461d8a7fbbacf030000', + `000000050139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e100000001018049d639e5a6980d1cd2392abcce41029cda74a1563523a202f09641cc2618f80000000101b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba0000000101ca5b4abdf9eec1f8e2d12c187d41ddd054c81979cae9e8ee9f4ecab901cac5b60000000101ef637606f3144ee46343ba4a25c261b5c400ade88528e876f3deababa22a444900000001010000000103290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e5630000000501${aliceSignature}01${bobSignature}01${carolSignature}0000`, 'hex', ); @@ -1691,16 +1743,6 @@ test.skip('Rotate signers with multisig prover encoded data', async () => { ], }); - const newSignersHash = getSignersHash( - [ - { signer: ALICE_PUB_KEY, weight: 1 }, - { signer: BOB_PUB_KEY, weight: 1 }, - { signer: CAROL_PUB_KEY, weight: 1 }, - ], - 3, - '290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563', - ); - assertAccount(await contract.getAccountWithKvs(), { balance: 0n, kvs: [ diff --git a/tests/helpers.ts b/tests/helpers.ts index d5a173c..86181c3 100644 --- a/tests/helpers.ts +++ b/tests/helpers.ts @@ -2,7 +2,6 @@ import fs from 'fs'; import { UserSecretKey } from '@multiversx/sdk-wallet/out'; import createKeccakHash from 'keccak'; import { e, Encodable } from 'xsuite'; -import { Buffer } from 'buffer'; export const MOCK_CONTRACT_ADDRESS_1: string = 'erd1qqqqqqqqqqqqqpgqd77fnev2sthnczp2lnfx0y5jdycynjfhzzgq6p3rax'; export const MOCK_CONTRACT_ADDRESS_2: string = 'erd1qqqqqqqqqqqqqpgq7ykazrzd905zvnlr88dpfw06677lxe9w0n4suz00uh'; From 2f0adb6295daff6981bbdc1c0d971b6ea7bf8b18 Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Fri, 22 Nov 2024 10:52:53 +0200 Subject: [PATCH 6/8] Fix remaining gateway & governance contract issues. --- gateway/src/auth.rs | 13 +++++--- gateway/src/lib.rs | 16 +++++----- governance/src/lib.rs | 54 +++++++++++----------------------- tests/gmp/gateway.test.ts | 23 ++++++++------- tests/governance.test.ts | 62 +-------------------------------------- 5 files changed, 47 insertions(+), 121 deletions(-) diff --git a/gateway/src/auth.rs b/gateway/src/auth.rs index b2bae7a..3234ff2 100644 --- a/gateway/src/auth.rs +++ b/gateway/src/auth.rs @@ -169,10 +169,15 @@ pub trait AuthModule: events::Events { ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); - // Will nest encode struct fields - signers - .top_encode(&mut encoded) - .unwrap_or_else(|_| sc_panic!("Could not encode weighted signers")); + signers.signers + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode signers")); + signers.threshold + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode threshold")); + signers.nonce + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode nonce")); self.crypto().keccak256(encoded) } diff --git a/gateway/src/lib.rs b/gateway/src/lib.rs index 386f249..0561bc5 100644 --- a/gateway/src/lib.rs +++ b/gateway/src/lib.rs @@ -180,13 +180,15 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events cross_chain_id .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode cross chain id")); - source_address .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode source address")); - - encoded.append(contract_address.as_managed_buffer()); - encoded.append(payload_hash.as_managed_buffer()); + contract_address + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode contract address")); + payload_hash + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode payload hash")); self.crypto().keccak256(encoded) } @@ -231,11 +233,7 @@ pub trait Gateway: auth::AuthModule + operator::OperatorModule + events::Events } #[view(isMessageExecuted)] - fn is_message_executed( - &self, - source_chain: ManagedBuffer, - message_id: ManagedBuffer, - ) -> bool { + fn is_message_executed(&self, source_chain: ManagedBuffer, message_id: ManagedBuffer) -> bool { let cross_chain_id = CrossChainId { source_chain, message_id, diff --git a/governance/src/lib.rs b/governance/src/lib.rs index 9e7fdca..056563c 100644 --- a/governance/src/lib.rs +++ b/governance/src/lib.rs @@ -90,52 +90,32 @@ pub trait Governance: events::Events { DecodedCallData::::top_decode(call_data) .unwrap_or_else(|_| sc_panic!("Could not decode call data")); - let gas_left = self.blockchain().get_gas_left(); - - require!( - gas_left > EXECUTE_PROPOSAL_CALLBACK_GAS + KEEP_EXTRA_GAS, - "Not enough gas left for async call" - ); - - let mut gas_limit = gas_left - EXECUTE_PROPOSAL_CALLBACK_GAS - KEEP_EXTRA_GAS; - - require!( - gas_limit >= decoded_call_data.min_gas_limit, - "Insufficient gas for execution" - ); - let caller = self.blockchain().get_caller(); let mut extra_gas_for_callback = EXECUTE_PROPOSAL_CALLBACK_GAS; let payments = self.call_value().any_payment(); - match payments.as_refs() { - EgldOrMultiEsdtPaymentRefs::Egld(egld_value) => { - if native_value > 0 { - require!(egld_value >= &native_value, "Not enough egld sent"); - } - } - EgldOrMultiEsdtPaymentRefs::MultiEsdt(payments) => { - if native_value > 0 { - require!(payments.is_empty(), "No egld payment sent"); - } + if let EgldOrMultiEsdtPaymentRefs::MultiEsdt(payments) = payments.as_refs() { + // Reserve extra gas for callback to make sure we can send back the tokens instead of async call error + let gas_for_payments = + EXECUTE_PROPOSAL_CALLBACK_GAS_PER_PAYMENT * payments.len() as u64; - let gas_for_payments = EXECUTE_PROPOSAL_CALLBACK_GAS_PER_PAYMENT * payments.len() as u64; + extra_gas_for_callback += gas_for_payments; + } - require!( - gas_limit >= gas_for_payments, - "Not enough gas left for async call payments" - ); + let gas_left = self.blockchain().get_gas_left(); - gas_limit -= gas_for_payments; - extra_gas_for_callback += gas_for_payments; - } - } + require!( + gas_left > extra_gas_for_callback + KEEP_EXTRA_GAS + decoded_call_data.min_gas_limit, + "Insufficient gas for execution" + ); + + let gas_limit = gas_left - extra_gas_for_callback - KEEP_EXTRA_GAS; self.send() .contract_call::<()>(target, decoded_call_data.endpoint_name) - .with_any_payment(payments.clone()) + .with_egld_transfer(native_value) .with_raw_arguments(decoded_call_data.arguments.into()) .with_gas_limit(gas_limit) .async_call_promise() @@ -277,12 +257,12 @@ pub trait Governance: events::Events { ) -> ManagedByteArray { let mut encoded = ManagedBuffer::new(); - encoded.append(target.as_managed_buffer()); - + target + .dep_encode(&mut encoded) + .unwrap_or_else(|_| sc_panic!("Could not encode target")); call_data .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode call data")); - native_value .dep_encode(&mut encoded) .unwrap_or_else(|_| sc_panic!("Could not encode native value")); diff --git a/tests/gmp/gateway.test.ts b/tests/gmp/gateway.test.ts index 059047b..ccb8c6e 100644 --- a/tests/gmp/gateway.test.ts +++ b/tests/gmp/gateway.test.ts @@ -1373,9 +1373,17 @@ describe('View functions', () => { data.toTopU8A(), ])); - const spoofedWeight = 198540501017381061748170477501535016683251460902464757637395385927941042274417159n; + // Spoof by top encoding Carol pub key in the weight of bob, without actually having the carol pub key + // in the signer set + const spoofedWeight = Buffer.concat([ + Buffer.from('06', 'hex'), // weight of bob + Buffer.from(CAROL_PUB_KEY, 'hex'), // carol pub key + Buffer.from('07', 'hex'), // weight of carol + ]).toString('hex'); + + // Omit carol from defaultWeightedSigners since we are trying to spoof it using the weight of bob const signers = [ - { signer: ALICE_PUB_KEY, weight: 5n }, + { signer: ALICE_PUB_KEY, weight: '05' }, { signer: BOB_PUB_KEY, weight: spoofedWeight, @@ -1383,14 +1391,9 @@ describe('View functions', () => { ]; let dataForSignersHashToSpoof = Buffer.concat([ ...signers.map(signer => { - let weightHex = signer.weight.toString(16); - if (weightHex.length % 2) { - weightHex = '0' + weightHex; - } - return Buffer.concat([ Buffer.from(signer.signer, 'hex'), - Buffer.from(weightHex, 'hex'), + Buffer.from(signer.weight, 'hex'), ]); }), Buffer.from('0a', 'hex'), // threshold @@ -1399,7 +1402,7 @@ describe('View functions', () => { const spoofedWeightedSigners = e.Tuple( e.List( e.Tuple(e.TopBuffer(ALICE_PUB_KEY), e.U(5)), - e.Tuple(e.TopBuffer(BOB_PUB_KEY), e.U(spoofedWeight)), + e.Tuple(e.TopBuffer(BOB_PUB_KEY), e.Buffer(spoofedWeight)), // use custom spoofed top encoded hex instead of weight ), e.U(10), e.TopBuffer(getKeccak256Hash('nonce1')), @@ -1408,7 +1411,7 @@ describe('View functions', () => { // Hash can not be the same assert(getKeccak256Hash(dataForSignersHashToSpoof) != defaultSignersHash.toString('hex')); - // Bob spoofed signature, but it will not work + // Bob spoofed signature, but it will not work since we use nested encoding in contract which can not be spoofed await world.query({ callee: contract, funcName: 'validateProof', diff --git a/tests/governance.test.ts b/tests/governance.test.ts index f72ff3e..dc77567 100644 --- a/tests/governance.test.ts +++ b/tests/governance.test.ts @@ -202,55 +202,6 @@ test('Execute proposal errors', async () => { }).assertFail({ code: 4, message: 'Could not decode call data' }); }); -test('Execute proposal esdt errors', async () => { - await deployContract(); - - // Increase timestamp so finalize_time_lock passes - await world.setCurrentBlockInfo({ timestamp: 1 }); - - const callData = e.TopBuffer(e.Tuple( - e.Str('function'), - e.List(), - e.U64(0), // min gas limit - ).toTopU8A()); - - const proposalHashWithEgld = getProposalHash(gateway, callData, e.U(1_000)); - - // Mock hash - await contract.setAccount({ - ...await contract.getAccountWithKvs(), - kvs: [ - ...baseKvs(), - - e.kvs.Mapper('time_lock_eta', proposalHashWithEgld).Value(e.U64(1)), - ], - }); - - await deployer.callContract({ - callee: contract, - gasLimit: 100_000_000, - funcName: 'executeProposal', - value: 999, - funcArgs: [ - gateway, - callData, - e.U(1_000), - ], - }).assertFail({ code: 4, message: 'Not enough egld sent' }); - - await deployer.callContract({ - callee: contract, - gasLimit: 100_000_000, - funcName: 'executeProposal', - funcArgs: [ - gateway, - callData, - e.U(1_000), - ], - esdts: [{ id: TOKEN_ID, amount: 1_000 }], - }).assertFail({ code: 4, message: 'No egld payment sent' }); -}); - test('Execute proposal upgrade gateway', async () => { await deployContract(); @@ -282,17 +233,6 @@ test('Execute proposal upgrade gateway', async () => { // Increase timestamp so finalize_time_lock passes await world.setCurrentBlockInfo({ timestamp: 1 }); - await deployer.callContract({ - callee: contract, - gasLimit: 30_000_000, - funcName: 'executeProposal', - funcArgs: [ - gateway, - callData, - e.U(0), - ], - }).assertFail({ code: 4, message: 'Not enough gas left for async call' }); - await deployer.callContract({ callee: contract, gasLimit: 50_000_000, @@ -424,7 +364,7 @@ test('Execute proposal upgrade gateway esdt error', async () => { e.U(0), ], esdts: [{ id: TOKEN_ID, amount: 1_000 }], - }).assertFail({ code: 4, message: 'Not enough gas left for async call payments' }); + }).assertFail({ code: 4, message: 'Insufficient gas for execution' }); await deployer.callContract({ callee: contract, From 790b28c73802b38e71bc0680e00b286c6ab262bb Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Fri, 22 Nov 2024 15:28:32 +0200 Subject: [PATCH 7/8] Add test for governance sending esdt token. --- tests/governance.test.ts | 88 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/tests/governance.test.ts b/tests/governance.test.ts index dc77567..fc69bd6 100644 --- a/tests/governance.test.ts +++ b/tests/governance.test.ts @@ -202,6 +202,93 @@ test('Execute proposal errors', async () => { }).assertFail({ code: 4, message: 'Could not decode call data' }); }); +test('Execute proposal esdt', async () => { + await deployContract(); + + const user = await world.createWallet(); + + const callData = e.TopBuffer(e.Tuple( + e.Str('MultiESDTNFTTransfer'), + e.List( + e.Buffer(user.toTopU8A()), + e.Buffer(e.U32(1).toTopU8A()), + e.Str(TOKEN_ID), + e.Buffer(e.U64(0).toTopU8A()), + e.Buffer(e.U(1_000).toTopU8A()), + ), // arguments to MultiESDTNFTTransfer function + e.U64(10_000_000), // min gas limit + ).toTopU8A()); + + const proposalHash = getProposalHash(contract, callData, e.U(0)); + + // Mock hash + await contract.setAccount({ + ...await contract.getAccountWithKvs(), + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + ], + }); + // Increase timestamp so finalize_time_lock passes + await world.setCurrentBlockInfo({ timestamp: 1 }); + + // Async call actually fails + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'executeProposal', + funcArgs: [ + contract, + callData, + e.U(0), + ], + }); + + // Time lock eta was NOT deleted + assertAccount(await contract.getAccountWithKvs(), { + balance: 0n, + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + ], + }); + + // Assert deployer still has the tokens + assertAccount(await deployer.getAccountWithKvs(), { + kvs: [ + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), + ], + }); + + // Deployer needs to send correct tokens + await deployer.callContract({ + callee: contract, + gasLimit: 200_000_000, + funcName: 'executeProposal', + funcArgs: [ + contract, + callData, + e.U(0), + ], + esdts: [{ id: TOKEN_ID, amount: 1_000 }], + }); + + // Time lock eta was deleted + assertAccount(await contract.getAccountWithKvs(), { + balance: 0n, + kvs: baseKvs(), + }); + + // Assert user received tokens + assertAccount(await user.getAccountWithKvs(), { + kvs: [ + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), + ], + }); +}); + test('Execute proposal upgrade gateway', async () => { await deployContract(); @@ -324,7 +411,6 @@ test('Execute proposal upgrade gateway error', async () => { }); }); - test('Execute proposal upgrade gateway esdt error', async () => { await deployContract(); From 4b9efc1f7b31da9e04cb096a794cf2a9b0f8212d Mon Sep 17 00:00:00 2001 From: Rares <6453351+raress96@users.noreply.github.com> Date: Mon, 25 Nov 2024 12:14:18 +0200 Subject: [PATCH 8/8] Implement refund token functionality for governance in case of execute proposal failure. --- governance/src/lib.rs | 44 ++++++++++- governance/wasm/src/lib.rs | 6 +- tests/governance.test.ts | 153 ++++++++++++++++++++++++++++++++++--- 3 files changed, 188 insertions(+), 15 deletions(-) diff --git a/governance/src/lib.rs b/governance/src/lib.rs index 056563c..1505dd3 100644 --- a/governance/src/lib.rs +++ b/governance/src/lib.rs @@ -31,6 +31,12 @@ pub struct ExecutePayload { pub eta: u64, } +#[derive(TypeAbi, TopDecode, TopEncode, NestedDecode, NestedEncode, Clone)] +pub struct EgldOrEsdtToken { + pub token_identifier: EgldOrEsdtTokenIdentifier, + pub token_nonce: u64, +} + const EXECUTE_PROPOSAL_CALLBACK_GAS: u64 = 10_000_000; const EXECUTE_PROPOSAL_CALLBACK_GAS_PER_PAYMENT: u64 = 2_000_000; // This is overkill, but the callback should be prevented from failing at all costs @@ -168,6 +174,14 @@ pub trait Governance: events::Events { self.process_command(execute_payload); } + #[endpoint(withdrawRefundToken)] + fn withdraw_refund_token(&self, token: EgldOrEsdtToken) { + let caller = self.blockchain().get_caller(); + let value = self.refund_token(&caller, token.clone()).take(); + + self.send().direct_non_zero(&caller, &token.token_identifier, token.token_nonce, &value); + } + fn process_command(&self, execute_payload: ExecutePayload) { let proposal_hash = self.get_proposal_hash( &execute_payload.target, @@ -294,10 +308,28 @@ pub trait Governance: events::Events { ManagedAsyncCallResult::Err(err) => { match payments { EgldOrMultiEsdtPayment::Egld(egld_value) => { - self.send().direct_non_zero_egld(&caller, &egld_value); + self.refund_token( + &caller, + EgldOrEsdtToken { + token_identifier: EgldOrEsdtTokenIdentifier::egld(), + token_nonce: 0, + }, + ) + .update(|old| *old += &egld_value); } EgldOrMultiEsdtPayment::MultiEsdt(esdts) => { - self.send().direct_multi(&caller, &esdts); + for esdt in esdts.iter() { + self.refund_token( + &caller, + EgldOrEsdtToken { + token_identifier: EgldOrEsdtTokenIdentifier::esdt( + esdt.token_identifier, + ), + token_nonce: esdt.token_nonce, + }, + ) + .update(|old| *old += &esdt.amount); + } } } @@ -352,6 +384,14 @@ pub trait Governance: events::Events { hash: &ManagedByteArray, ) -> SingleValueMapper; + #[view(getRefundToken)] + #[storage_mapper("refund_token")] + fn refund_token( + &self, + user: &ManagedAddress, + token: EgldOrEsdtToken, + ) -> SingleValueMapper; + #[proxy] fn gateway_proxy(&self, sc_address: ManagedAddress) -> gateway::Proxy; } diff --git a/governance/wasm/src/lib.rs b/governance/wasm/src/lib.rs index 4dced8b..ed175ae 100644 --- a/governance/wasm/src/lib.rs +++ b/governance/wasm/src/lib.rs @@ -6,10 +6,10 @@ // Init: 1 // Upgrade: 1 -// Endpoints: 11 +// Endpoints: 13 // Async Callback (empty): 1 // Promise callbacks: 1 -// Total number of exported functions: 15 +// Total number of exported functions: 17 #![no_std] @@ -24,6 +24,7 @@ multiversx_sc_wasm_adapter::endpoints! { executeProposal => execute_proposal withdraw => withdraw execute => execute + withdrawRefundToken => withdraw_refund_token getProposalEta => get_proposal_eta gateway => gateway getMinimumTimeLockDelay => minimum_time_lock_delay @@ -32,6 +33,7 @@ multiversx_sc_wasm_adapter::endpoints! { getGovernanceChainHash => governance_chain_hash getGovernanceAddressHash => governance_address_hash getTimeLockEta => time_lock_eta + getRefundToken => refund_token execute_proposal_callback => execute_proposal_callback ) } diff --git a/tests/governance.test.ts b/tests/governance.test.ts index fc69bd6..bf4e422 100644 --- a/tests/governance.test.ts +++ b/tests/governance.test.ts @@ -24,7 +24,7 @@ beforeEach(async () => { deployer = await world.createWallet({ balance: 10_000_000_000n, kvs: [ - e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000, nonce: 1 }]), ], }); }); @@ -213,7 +213,7 @@ test('Execute proposal esdt', async () => { e.Buffer(user.toTopU8A()), e.Buffer(e.U32(1).toTopU8A()), e.Str(TOKEN_ID), - e.Buffer(e.U64(0).toTopU8A()), + e.Buffer(e.U64(1).toTopU8A()), e.Buffer(e.U(1_000).toTopU8A()), ), // arguments to MultiESDTNFTTransfer function e.U64(10_000_000), // min gas limit @@ -258,7 +258,7 @@ test('Execute proposal esdt', async () => { // Assert deployer still has the tokens assertAccount(await deployer.getAccountWithKvs(), { kvs: [ - e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000, nonce: 1 }]), ], }); @@ -272,7 +272,7 @@ test('Execute proposal esdt', async () => { callData, e.U(0), ], - esdts: [{ id: TOKEN_ID, amount: 1_000 }], + esdts: [{ id: TOKEN_ID, amount: 1_000, nonce: 1 }], }); // Time lock eta was deleted @@ -284,7 +284,7 @@ test('Execute proposal esdt', async () => { // Assert user received tokens assertAccount(await user.getAccountWithKvs(), { kvs: [ - e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000, nonce: 1 }]), ], }); }); @@ -395,14 +395,25 @@ test('Execute proposal upgrade gateway error', async () => { ], }); // async call actually fails - // Time lock eta was NOT deleted + // Time lock eta was NOT deleted and refund token was created let kvs = await contract.getAccountWithKvs(); assertAccount(kvs, { - balance: 0n, + balance: 1_000, // EGLD still in contract kvs: [ ...baseKvs(), e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + e.kvs.Mapper('refund_token', deployer, e.Tuple(e.Str('EGLD'), e.U64(0))).Value(e.U(1_000)), + ], + }); + + // Deployer can withdraw his funds in case of failure + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'withdrawRefundToken', + funcArgs: [ + e.Tuple(e.Str('EGLD'), e.U64(0)), ], }); @@ -449,7 +460,7 @@ test('Execute proposal upgrade gateway esdt error', async () => { callData, e.U(0), ], - esdts: [{ id: TOKEN_ID, amount: 1_000 }], + esdts: [{ id: TOKEN_ID, amount: 1_000, nonce: 1 }], }).assertFail({ code: 4, message: 'Insufficient gas for execution' }); await deployer.callContract({ @@ -461,10 +472,10 @@ test('Execute proposal upgrade gateway esdt error', async () => { callData, e.U(0), ], - esdts: [{ id: TOKEN_ID, amount: 1_000 }], + esdts: [{ id: TOKEN_ID, amount: 500, nonce: 1 }], }); // async call actually fails - // Time lock eta was NOT deleted + // Time lock eta was NOT deleted and refund token was created let kvs = await contract.getAccountWithKvs(); assertAccount(kvs, { balance: 0n, @@ -472,15 +483,135 @@ test('Execute proposal upgrade gateway esdt error', async () => { ...baseKvs(), e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + e.kvs.Mapper('refund_token', deployer, e.Tuple(e.Str(TOKEN_ID), e.U64(1))).Value(e.U(500)), + + e.kvs.Esdts([{ id: TOKEN_ID, amount: 500, nonce: 1 }]), // esdt still in contract + ], + }); + + // Try to execute again + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'executeProposal', + funcArgs: [ + gateway, + callData, + e.U(0), + ], + esdts: [{ id: TOKEN_ID, amount: 500, nonce: 1 }], + }); // async call actually fails + + // Amount was added to refund token + assertAccount(await contract.getAccountWithKvs(), { + balance: 0n, + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('time_lock_eta', proposalHash).Value(e.U64(1)), + e.kvs.Mapper('refund_token', deployer, e.Tuple(e.Str(TOKEN_ID), e.U64(1))).Value(e.U(1_000)), + + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000, nonce: 1 }]), + ], + }); + + // Deployer can withdraw his funds in case of failure + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'withdrawRefundToken', + funcArgs: [ + e.Tuple(e.Str(TOKEN_ID), e.U64(1)), ], }); assertAccount(await deployer.getAccountWithKvs(), { balance: 10_000_000_000n, kvs: [ - e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000 }]), // got esdt back + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000, nonce: 1 }]), // got esdt back + ], + }); +}); + +test('Withdraw refund token', async () => { + await deployContract(); + + // Will do nothing + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'withdrawRefundToken', + funcArgs: [ + e.Tuple(e.Str(TOKEN_ID), e.U64(1)), ], }); + + // Nothing has changed + assertAccount(await deployer.getAccountWithKvs(), { + balance: 10_000_000_000n, + }); + assertAccount(await contract.getAccountWithKvs(), { + balance: 0n, + kvs: baseKvs(), + }); + + // Mock refund tokens + await contract.setAccount({ + ...(await contract.getAccountWithKvs()), + balance: 1_000n, + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('refund_token', deployer, e.Tuple(e.Str(TOKEN_ID), e.U64(1))).Value(e.U(1_000)), + e.kvs.Mapper('refund_token', deployer, e.Tuple(e.Str('EGLD'), e.U64(0))).Value(e.U(1_000)), + + e.kvs.Esdts([{ id: TOKEN_ID, amount: 1_000, nonce: 1 }]), + ], + }); + + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'withdrawRefundToken', + funcArgs: [ + e.Tuple(e.Str(TOKEN_ID), e.U64(1)), + ], + }); + + assertAccount(await deployer.getAccountWithKvs(), { + balance: 10_000_000_000n, + kvs: [ + e.kvs.Esdts([{ id: TOKEN_ID, amount: 2_000, nonce: 1 }]), // got esdt back + ], + }); + assertAccount(await contract.getAccountWithKvs(), { + balance: 1_000n, + kvs: [ + ...baseKvs(), + + e.kvs.Mapper('refund_token', deployer, e.Tuple(e.Str('EGLD'), e.U64(0))).Value(e.U(1_000)), + ], + }); + + await deployer.callContract({ + callee: contract, + gasLimit: 50_000_000, + funcName: 'withdrawRefundToken', + funcArgs: [ + e.Tuple(e.Str('EGLD'), e.U64(0)), + ], + }); + + assertAccount(await deployer.getAccountWithKvs(), { + balance: 10_000_001_000n, // got egld back + kvs: [ + e.kvs.Esdts([{ id: TOKEN_ID, amount: 2_000, nonce: 1 }]), + ], + }); + assertAccount(await contract.getAccountWithKvs(), { + balance: 0, + kvs: baseKvs(), + }); }); test('Withdraw', async () => {