From a3bbac6c5c4c97cca2e33411428ba689b1b7ec33 Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Fri, 8 Dec 2023 11:13:15 +0100 Subject: [PATCH 1/7] implement timelock --- src/core/timelock.cairo | 326 +++++++++++++++--- src/lib.cairo | 2 + src/mocks/simple_contract_mock.cairo | 28 ++ src/utils/hash.cairo | 25 ++ src/utils/math.cairo | 2 +- .../accept_admin/test_accept_admin.cairo | 51 +++ .../test_cancel_transaction.cairo | 96 ++++++ .../execute_transaction.tree | 2 +- .../test_execute_transaction.cairo | 185 ++++++++++ .../queue_transaction/queue_transaction.tree | 2 +- .../test_queue_transaction.cairo | 165 +++++++++ .../timelock/set_delay/test_set_delay.cairo | 66 ++++ .../test_set_pending_admin.cairo | 48 +++ tests/tests_lib.cairo | 45 ++- 14 files changed, 990 insertions(+), 53 deletions(-) create mode 100644 src/mocks/simple_contract_mock.cairo create mode 100644 src/utils/hash.cairo diff --git a/src/core/timelock.cairo b/src/core/timelock.cairo index 00b9373..fd589b2 100644 --- a/src/core/timelock.cairo +++ b/src/core/timelock.cairo @@ -3,44 +3,72 @@ use starknet::ContractAddress; #[starknet::interface] trait ITimelock { - fn set_delay(ref self: TContractState, _delay: u256); + fn set_delay(ref self: TContractState, delay: u256); + fn get_delay(self: @TContractState) -> u256; fn accept_admin(ref self: TContractState); - fn set_pending_admin(ref self: TContractState, _pending_admin: ContractAddress); + fn get_admin(self: @TContractState) -> ContractAddress; + fn set_pending_admin(ref self: TContractState, pending_admin: ContractAddress); + fn get_pending_admin(self: @TContractState) -> ContractAddress; + + fn get_grace_period(self: @TContractState) -> u256; + fn get_minimum_delay(self: @TContractState) -> u256; + fn get_maximum_delay(self: @TContractState) -> u256; + + fn get_tx_status(self: @TContractState, tx_hash: felt252) -> bool; + fn queue_transaction( ref self: TContractState, - _target: ContractAddress, - _value: u256, - _signature: felt252, - _data: Span, - _eta: u256, + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256, ) -> felt252; + fn cancel_transaction( ref self: TContractState, - _target: ContractAddress, - _value: u256, - _signature: felt252, - _data: Span, - _eta: u256, + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256, ); + fn execute_transaction( ref self: TContractState, - _target: ContractAddress, - _value: u256, - _signature: felt252, - _data: Span, - _eta: u256, - ) -> felt252; + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256, + ); } #[starknet::contract] mod Timelock { - use starknet::ContractAddress; + use core::traits::Into; + use core::zeroable::Zeroable; + use core::array::ArrayTrait; + use core::starknet::event::EventEmitter; + use core::option::OptionTrait; + use core::serde::Serde; + use core::traits::TryInto; + use core::box::BoxTrait; + use starknet::{ + ContractAddress, contract_address_const, get_caller_address, get_contract_address, + get_block_timestamp, call_contract_syscall + }; + use poseidon::PoseidonTrait; + use shisui::utils::hash::ISpanFelt252Hash; + use core::hash::HashStateTrait; + use core::hash::HashStateExTrait; + use starknet::SyscallResultTrait; - const MINIMUM_DELAY: u256 = 172_800; // 2 days - const MAXIMUM_DELAY: u256 = 1_296_000; // 15 days - const GRACE_PERIOD: u256 = 1_209_600; // 14 days + const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days + const MAXIMUM_DELAY: u256 = consteval_int!(15 * 24 * 60 * 60); // 15 days + const GRACE_PERIOD: u256 = consteval_int!(14 * 24 * 60 * 60); // 14 days + // ************************************************************************* + // STORAGE + // ************************************************************************* #[storage] struct Storage { admin: ContractAddress, @@ -49,6 +77,74 @@ mod Timelock { queued_transactions: LegacyMap::, } + // ************************************************************************* + // EVENT + // ************************************************************************* + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + NewAdmin: NewAdmin, + NewPendingAdmin: NewPendingAdmin, + NewDelay: NewDelay, + CancelTransaction: CancelTransaction, + ExecuteTransaction: ExecuteTransaction, + QueueTransaction: QueueTransaction + } + + #[derive(Drop, starknet::Event)] + struct NewAdmin { + #[key] + new_admin: ContractAddress + } + + #[derive(Drop, starknet::Event)] + struct NewPendingAdmin { + #[key] + new_pending_admin: ContractAddress + } + + #[derive(Drop, starknet::Event)] + struct NewDelay { + #[key] + new_delay: u256 + } + + #[derive(Drop, starknet::Event)] + struct CancelTransaction { + #[key] + tx_hash: felt252, + #[key] + target: ContractAddress, + signature: felt252, + input_data: Span, + eta: u256 + } + + #[derive(Drop, starknet::Event)] + struct ExecuteTransaction { + #[key] + tx_hash: felt252, + #[key] + target: ContractAddress, + signature: felt252, + input_data: Span, + eta: u256 + } + + #[derive(Drop, starknet::Event)] + struct QueueTransaction { + #[key] + tx_hash: felt252, + #[key] + target: ContractAddress, + signature: felt252, + input_data: Span, + eta: u256 + } + + // ************************************************************************* + // ERRORS + // ************************************************************************* mod Errors { const Timelock__DelayMustExceedMininumDelay: felt252 = 'Delay must exceed mininum delay'; const Timelock__DelayMustNotExceedMaximumDelay: felt252 = 'Delay must under maximum delay'; @@ -61,56 +157,188 @@ mod Timelock { const Timelock__TxStillLocked: felt252 = 'Tx still locked'; const Timelock__TxExpired: felt252 = 'Tx expired'; const Timelock__TxReverted: felt252 = 'Tx reverted'; + const Timelock__ZeroAddressAdmin: felt252 = 'Admin is the zero address'; + const Timelock_ZeroAddressCaller: felt252 = 'Caller is the zero address'; } - + // ************************************************************************* + // CONSTRUCTOR + // ************************************************************************* #[constructor] - fn constructor(ref self: ContractState, _delay: u256, _admin: ContractAddress) {} + fn constructor(ref self: ContractState, _delay: u256, _admin: ContractAddress) { + self.is_valid_delay(_delay); + assert(_admin.is_non_zero(), Errors::Timelock__ZeroAddressAdmin); + self.admin.write(_admin); + self.delay.write(_delay); + } + // ************************************************************************* + // INTERNAL FUNCTIONS + // ************************************************************************* #[external(v0)] impl TimelockImpl of super::ITimelock { - fn set_delay(ref self: ContractState, _delay: u256) {} + fn get_delay(self: @ContractState) -> u256 { + self.delay.read() + } + + fn get_admin(self: @ContractState) -> ContractAddress { + self.admin.read() + } - fn accept_admin(ref self: ContractState) {} + fn get_pending_admin(self: @ContractState) -> ContractAddress { + self.pending_admin.read() + } - fn set_pending_admin(ref self: ContractState, _pending_admin: ContractAddress) {} + fn get_grace_period(self: @ContractState) -> u256 { + GRACE_PERIOD + } + + fn get_minimum_delay(self: @ContractState) -> u256 { + MINIMUM_DELAY + } + + fn get_maximum_delay(self: @ContractState) -> u256 { + MAXIMUM_DELAY + } + + fn get_tx_status(self: @ContractState, tx_hash: felt252) -> bool { + self.queued_transactions.read(tx_hash) + } + + fn set_delay(ref self: ContractState, delay: u256) { + self.is_valid_delay(delay); + assert(get_caller_address() == get_contract_address(), Errors::Timelock__TimelockOnly); + + self.delay.write(delay); + self.emit(NewDelay { new_delay: delay }); + } + + fn accept_admin(ref self: ContractState) { + let caller = get_caller_address(); + assert(caller == self.pending_admin.read(), Errors::Timelock__PendingAdminOnly); + + self.admin.write(caller); + self.pending_admin.write(contract_address_const::<0>()); + self.emit(NewAdmin { new_admin: caller }); + } + + fn set_pending_admin(ref self: ContractState, pending_admin: ContractAddress) { + assert(get_caller_address() == get_contract_address(), Errors::Timelock__TimelockOnly); + + self.pending_admin.write(pending_admin); + self.emit(NewPendingAdmin { new_pending_admin: pending_admin }); + } fn queue_transaction( ref self: ContractState, - _target: ContractAddress, - _value: u256, - _signature: felt252, - _data: Span, - _eta: u256, + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256 ) -> felt252 { - return '0'; + self.admin_only(); + let block_time_stamp: u256 = get_block_timestamp().into(); + let delay = self.delay.read(); + + assert( + eta >= block_time_stamp + delay && eta <= block_time_stamp + delay + GRACE_PERIOD, + Errors::Timelock__ETAMustSatisfyDelay + ); + + let tx_hash = self.get_hash(target, signature, data, eta); + assert(!self.queued_transactions.read(tx_hash), Errors::Timelock__TxAlreadyQueued); + + self.queued_transactions.write(tx_hash, true); + self + .emit( + QueueTransaction { + tx_hash, target: target, signature: signature, input_data: data, eta: eta + } + ); + tx_hash } fn cancel_transaction( ref self: ContractState, - _target: ContractAddress, - _value: u256, - _signature: felt252, - _data: Span, - _eta: u256, - ) {} + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256 + ) { + self.admin_only(); + let tx_hash = self.get_hash(target, signature, data, eta); + assert(self.queued_transactions.read(tx_hash), Errors::Timelock__TxNoQueued); + + self.queued_transactions.write(tx_hash, false); + self + .emit( + CancelTransaction { + tx_hash, target: target, signature: signature, input_data: data, eta: eta + } + ); + } fn execute_transaction( ref self: ContractState, - _target: ContractAddress, - _value: u256, - _signature: felt252, - _data: Span, - _eta: u256, - ) -> felt252 { - return '0'; + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256 + ) { + self.admin_only(); + + let tx_hash = self.get_hash(target, signature, data, eta); + assert(self.queued_transactions.read(tx_hash), Errors::Timelock__TxNoQueued); + + assert(get_block_timestamp().into() >= eta, Errors::Timelock__TxStillLocked); + assert(get_block_timestamp().into() <= eta + GRACE_PERIOD, Errors::Timelock__TxExpired); + + self.queued_transactions.write(tx_hash, false); + match call_contract_syscall(target, signature, data) { + Result::Ok(return_data) => { + self + .emit( + ExecuteTransaction { + tx_hash, + target: target, + signature: signature, + input_data: data, + eta: eta + } + ); + }, + Result::Err(revert_reason) => { panic_with_felt252(Errors::Timelock__TxReverted); }, + } } } - + // ************************************************************************* + // INTERNAL FUNCTIONS + // ************************************************************************* #[generate_trait] impl InternalFunctions of InternalFunctionsTrait { - fn _is_valid_delay(_delay: u256) {} - fn _admin_only() {} + fn is_valid_delay(self: @ContractState, _delay: u256) { + assert(_delay > MINIMUM_DELAY, Errors::Timelock__DelayMustExceedMininumDelay); + assert(_delay < MAXIMUM_DELAY, Errors::Timelock__DelayMustNotExceedMaximumDelay); + } + + fn admin_only(self: @ContractState) { + assert(get_caller_address() == self.admin.read(), Errors::Timelock__AdminOnly) + } + + fn get_hash( + self: @ContractState, + target: ContractAddress, + signature: felt252, + data: Span, + eta: u256 + ) -> felt252 { + let mut hash_state = PoseidonTrait::new(); + hash_state = hash_state.update_with(target); + hash_state = hash_state.update_with(signature); + hash_state = hash_state.update_with(data.hash_span()); + hash_state = hash_state.update_with(eta); + hash_state.finalize() + } } } diff --git a/src/lib.cairo b/src/lib.cairo index 6c68565..448e704 100644 --- a/src/lib.cairo +++ b/src/lib.cairo @@ -26,10 +26,12 @@ mod utils { mod traits; mod math; mod shisui_math; + mod hash; } mod mocks { mod safety_transfer_mock; mod erc20_mock; + mod simple_contract_mock; } diff --git a/src/mocks/simple_contract_mock.cairo b/src/mocks/simple_contract_mock.cairo new file mode 100644 index 0000000..e5bde2c --- /dev/null +++ b/src/mocks/simple_contract_mock.cairo @@ -0,0 +1,28 @@ +#[starknet::interface] +trait ISimpleStorage { + fn set(ref self: TContractState, x: u8); + fn get(self: @TContractState) -> u8; +} + +#[starknet::contract] +mod SimpleStorage { + use starknet::get_caller_address; + use starknet::ContractAddress; + use debug::PrintTrait; + + #[storage] + struct Storage { + stored_data: u8 + } + + #[external(v0)] + impl SimpleStorage of super::ISimpleStorage { + fn set(ref self: ContractState, x: u8) { + assert(x < 200, 'Invalid int, must be <= 200'); + self.stored_data.write(x); + } + fn get(self: @ContractState) -> u8 { + self.stored_data.read() + } + } +} diff --git a/src/utils/hash.cairo b/src/utils/hash.cairo new file mode 100644 index 0000000..fb8f0a4 --- /dev/null +++ b/src/utils/hash.cairo @@ -0,0 +1,25 @@ +use hash::{LegacyHash, HashStateTrait, HashStateExTrait}; + +trait ISpanFelt252Hash { + fn hash_span(self: @T) -> felt252; +} + + +impl HashSpanFelt252 of ISpanFelt252Hash> { + fn hash_span(self: @Span) -> felt252 { + let mut call_data_state = LegacyHash::hash(0, *self); + call_data_state = LegacyHash::hash(call_data_state, (*self).len()); + call_data_state + } +} + +impl LegacyHashSpanFelt252 of LegacyHash> { + fn hash(mut state: felt252, mut value: Span) -> felt252 { + loop { + match value.pop_front() { + Option::Some(item) => { state = LegacyHash::hash(state, *item); }, + Option::None(_) => { break state; }, + }; + } + } +} diff --git a/src/utils/math.cairo b/src/utils/math.cairo index 034a0a1..1c81907 100644 --- a/src/utils/math.cairo +++ b/src/utils/math.cairo @@ -2,7 +2,7 @@ /// * `x` - The number to raise. /// * `n` - The exponent. /// # Returns -/// * `u128` - The result of x raised to the power of n. +/// * `u256` - The result of x raised to the power of n. fn pow(x: u256, n: u8) -> u256 { if n == 0 { 1 diff --git a/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo b/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo index 8b13789..cb90c00 100644 --- a/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo +++ b/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo @@ -1 +1,52 @@ +use core::zeroable::Zeroable; +use snforge_std::cheatcodes::events::EventFetcher; +use starknet::{ContractAddress, contract_address_const,}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock}; +use snforge_std::{ + start_prank, stop_prank, CheatTarget, spy_events, SpyOn, EventSpy, EventAssertions +}; +const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days + +// This test check accept admin with caller isn't timelock contract +// It calls timelock.accept_admin() +// The test expects to fails with error Pending admin only +#[test] +#[should_panic(expected: ('Pending admin only',))] +fn when_caller_not_timelock_contract_it_should_revert() { + let (timelock, _) = init(); + timelock.accept_admin(); +} + +// This test check accept admin with caller is pending admin +// It calls timelock.accept_admin() +// The test expects to succeed +#[test] +fn when_caller_is_pending_admin_it_should_work() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + // set pending admin + start_prank(CheatTarget::One(timelock_address), timelock_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock_address)); + // accept admin + start_prank(CheatTarget::One(timelock_address), admin_address); + let mut spy = spy_events(SpyOn::One(timelock_address)); + timelock.accept_admin(); + spy + .assert_emitted( + @array![ + ( + timelock_address, + Timelock::Event::NewAdmin(Timelock::NewAdmin { new_admin: admin_address }) + ) + ] + ); + assert(spy.events.len() == 0, 'There should be no events'); + assert(timelock.get_pending_admin().is_zero(), 'Pending admin not reset'); + assert(timelock.get_admin() == admin_address, 'Wrong admin set'); +} + +fn init() -> (ITimelockDispatcher, ContractAddress) { + tests::tests_lib::deploy_timelock_mock(MINIMUM_DELAY + 1, contract_address_const::<'admin'>()) +} diff --git a/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo b/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo index 8b13789..5833c20 100644 --- a/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo +++ b/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo @@ -1 +1,97 @@ +use snforge_std::cheatcodes::events::EventFetcher; +use starknet::{ContractAddress, contract_address_const,}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock}; +use poseidon::PoseidonTrait; +use shisui::utils::hash::ISpanFelt252Hash; +use core::hash::HashStateTrait; +use core::hash::HashStateExTrait; +use snforge_std::{ + start_prank, stop_prank, start_warp, CheatTarget, spy_events, SpyOn, EventSpy, EventAssertions +}; +const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days + +// This test check cancel transaction with caller isn't the admin address +// It calls timelock.cancel_transaction() +// The test expects to fails with error Pending admin only +#[test] +#[should_panic(expected: ('Admin only',))] +fn when_caller_not_admin_it_should_revert() { + let (timelock, _) = init(); + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 1; + + timelock.cancel_transaction(target, signature, data.span(), eta); +} + +// This test check cancel transaction with transaction not in queue +// It calls timelock.cancel_transaction() +// The test expects to fails with error Tx no queued +#[test] +#[should_panic(expected: ('Tx no queued',))] +fn when_caller_is_admin_and_tx_not_queue_it_should_revert() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + set_admin(timelock, timelock_address, admin_address); + + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 1; + + timelock.cancel_transaction(target, signature, data.span(), eta); +} + +// This test check cancel transaction with transaction in queue +// It calls timelock.cancel_transaction() +// The test expects to succeed +#[test] +fn when_caller_is_admin_and_tx_in_queue_it_should_works() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + set_admin(timelock, timelock_address, admin_address); + + start_warp(CheatTarget::One(timelock_address), 1000); + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 20000; + let tx_hash = 0x3d1394918f9a449d63ca74a79bdc7d573179ea887fae5a9896d1f93b6894384; + timelock.queue_transaction(target, signature, data.span(), eta); + let mut spy = spy_events(SpyOn::One(timelock_address)); + timelock.cancel_transaction(target, signature, data.span(), eta); + spy + .assert_emitted( + @array![ + ( + timelock_address, + Timelock::Event::CancelTransaction( + Timelock::CancelTransaction { + tx_hash, target, signature, input_data: data.span(), eta + } + ) + ) + ] + ); + assert(spy.events.len() == 0, 'There should be no events'); + assert(!timelock.get_tx_status(tx_hash), 'Tx still in queued'); +} + +fn set_admin( + timelock: ITimelockDispatcher, timelock_address: ContractAddress, admin_address: ContractAddress +) { + // set pending admin + start_prank(CheatTarget::One(timelock_address), timelock_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock_address)); + // accept admin + start_prank(CheatTarget::One(timelock_address), admin_address); + timelock.accept_admin(); +} + + +fn init() -> (ITimelockDispatcher, ContractAddress) { + tests::tests_lib::deploy_timelock_mock(MINIMUM_DELAY + 1, contract_address_const::<'admin'>()) +} diff --git a/tests/integration/core/timelock/execute_transaction/execute_transaction.tree b/tests/integration/core/timelock/execute_transaction/execute_transaction.tree index df8989a..ac65157 100644 --- a/tests/integration/core/timelock/execute_transaction/execute_transaction.tree +++ b/tests/integration/core/timelock/execute_transaction/execute_transaction.tree @@ -10,7 +10,7 @@ test_execute_transaction.cairo │ └── it should revert with Timelock__TxExpired └── when tx_hash is in queue AND current block timestamp is valid ├── when call is not successful - │ └── it should revert with Timelock__TxReverted + │ └── it should revert └── when call is successful ├── it should set tx_hash from queue to false ├── it should excute the transaction diff --git a/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo b/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo index 8b13789..22831ed 100644 --- a/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo +++ b/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo @@ -1 +1,186 @@ +use core::option::OptionTrait; +use core::traits::TryInto; +use core::traits::Into; +use snforge_std::cheatcodes::events::EventFetcher; +use starknet::{ContractAddress, contract_address_const,}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock}; +use shisui::mocks::simple_contract_mock::{ISimpleStorageDispatcher, ISimpleStorageDispatcherTrait}; +use poseidon::PoseidonTrait; +use shisui::utils::hash::ISpanFelt252Hash; +use core::hash::HashStateTrait; +use core::hash::HashStateExTrait; +use snforge_std::{ + start_prank, stop_prank, start_warp, stop_warp, CheatTarget, spy_events, SpyOn, EventSpy, + EventAssertions +}; +use serde::Serde; +use debug::PrintTrait; +const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days +const GRACE_PERIOD: u256 = consteval_int!(14 * 24 * 60 * 60); // 14 days + +// This test check execute transaction with caller isn't the admin address +// It calls timelock.execute_transaction() +// The test expects to fails with error Pending admin only +#[test] +#[should_panic(expected: ('Admin only',))] +fn when_caller_not_admin_it_should_revert() { + let (timelock, _,) = init(); + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 1; + + timelock.execute_transaction(target, signature, data.span(), eta); +} + +// This test check execute transaction with transaction not in queue +// It calls timelock.execute_transaction() +// The test expects to fails with error Tx no queued +#[test] +#[should_panic(expected: ('Tx no queued',))] +fn when_caller_is_admin_and_tx_not_queue_it_should_revert() { + let (timelock, _) = init(); + set_admin(timelock); + + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 1; + + timelock.execute_transaction(target, signature, data.span(), eta); +} + +// This test check execute transaction with block timestamp below eta +// It calls timelock.execute_transaction() +// The test expects to fails with error Tx still locked +#[test] +#[should_panic(expected: ('Tx still locked',))] +fn when_caller_is_admin_and_block_timestamp_below_eta_it_should_revert() { + let (timelock, simple_contract) = init(); + set_admin(timelock); + let (signature, data, eta) = queue_transaction( + timelock, simple_contract.contract_address, 5, MINIMUM_DELAY + 20000 + ); + timelock.execute_transaction(simple_contract.contract_address, signature, data, eta); +} + +// This test check execute transaction with block timestamp below eta +// It calls timelock.execute_transaction() +// The test expects to fails with error Tx expired +#[test] +#[should_panic(expected: ('Tx expired',))] +fn when_caller_is_admin_and_block_timestamp_above_eta_it_should_revert() { + let (timelock, simple_contract) = init(); + set_admin(timelock); + let (signature, data, eta) = queue_transaction( + timelock, simple_contract.contract_address, 5, MINIMUM_DELAY + 20000 + ); + start_warp( + CheatTarget::One(timelock.contract_address), + eta.try_into().unwrap() + GRACE_PERIOD.try_into().unwrap() + 1 + ); + + timelock.execute_transaction(simple_contract.contract_address, signature, data, eta); +} + +// This test check execute transaction with block timestamp valid, call invalid and unrecoverable error +// It calls timelock.execute_transaction() +// The test expects to fails +#[test] +#[should_panic] +fn when_caller_is_admin_and_block_timestamp_is_valid_and_call_not_valid_it_should_revert() { + //init contract + let (timelock, simple_contract) = init(); + set_admin(timelock); + let (signature, data, eta) = queue_transaction( + timelock, simple_contract.contract_address, 256, MINIMUM_DELAY + 20000 + ); + start_warp(CheatTarget::One(timelock.contract_address), eta.try_into().unwrap() + 1); + //execute transaction - should revert + timelock.execute_transaction(simple_contract.contract_address, signature, data, eta); +} + +// This test check execute transaction with block timestamp and call valid +// It calls timelock.execute_transaction() +// The test expects to succeed +#[test] +fn when_caller_is_admin_and_block_timestamp_is_valid_and_call_is_valid_it_should_work() { + let (timelock, simple_contract) = init(); + set_admin(timelock); + + let (signature, data, eta) = queue_transaction( + timelock, simple_contract.contract_address, 4, MINIMUM_DELAY + 20000 + ); + + let tx_hash = get_hash(simple_contract.contract_address, signature, data, eta); + start_warp(CheatTarget::One(timelock.contract_address), eta.try_into().unwrap() + 1); + let mut spy = spy_events(SpyOn::One(timelock.contract_address)); + timelock.execute_transaction(simple_contract.contract_address, signature, data, eta); + spy + .assert_emitted( + @array![ + ( + timelock.contract_address, + Timelock::Event::ExecuteTransaction( + Timelock::ExecuteTransaction { + tx_hash, + target: simple_contract.contract_address, + signature, + input_data: data, + eta + } + ) + ) + ] + ); + assert(simple_contract.get() == 4, 'Call to simple contract failed'); + assert(!timelock.get_tx_status(tx_hash), 'Tx still in queued'); +} + +fn init() -> (ITimelockDispatcher, ISimpleStorageDispatcher) { + let (timelock, timelock_address) = tests::tests_lib::deploy_timelock_mock( + MINIMUM_DELAY + 1, contract_address_const::<'admin'>() + ); + let address = tests::tests_lib::deploy_simple_storage_mock(); + let simple_storage = ISimpleStorageDispatcher { contract_address: address }; + (timelock, simple_storage) +} + +fn set_admin(timelock: ITimelockDispatcher) { + let admin_address = contract_address_const::<'admin'>(); + // set pending admin + start_prank(CheatTarget::One(timelock.contract_address), timelock.contract_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock.contract_address)); + // accept admin + start_prank(CheatTarget::One(timelock.contract_address), admin_address); + timelock.accept_admin(); +} + +fn queue_transaction( + timelock: ITimelockDispatcher, target: ContractAddress, data: felt252, eta: u256 +) -> (felt252, Span, u256) { + //set block timestamp + start_warp(CheatTarget::One(timelock.contract_address), 1000); + //define data + let signature = selector!("set"); + let mut call_data: Array = ArrayTrait::new(); + Serde::serialize(@data, ref call_data); + //queue transcation + timelock.queue_transaction(target, signature, call_data.span(), eta); + //set block timestamp + stop_warp(CheatTarget::One(timelock.contract_address)); + (signature, call_data.span(), eta) +} + +fn get_hash( + _target: ContractAddress, _signature: felt252, _data: Span, _eta: u256 +) -> felt252 { + let mut hash_state = PoseidonTrait::new(); + hash_state = hash_state.update_with(_target); + hash_state = hash_state.update_with(_signature); + hash_state = hash_state.update_with(_data.hash_span()); + hash_state = hash_state.update_with(_eta); + hash_state.finalize() +} diff --git a/tests/integration/core/timelock/queue_transaction/queue_transaction.tree b/tests/integration/core/timelock/queue_transaction/queue_transaction.tree index be4da2b..8f5045a 100644 --- a/tests/integration/core/timelock/queue_transaction/queue_transaction.tree +++ b/tests/integration/core/timelock/queue_transaction/queue_transaction.tree @@ -2,7 +2,7 @@ test_queue_transaction.cairo ├── when caller is not the admin │ └── it should revert with Timelock__AdminOnly └── when caller is the admin - ├── when eta is lower than the current block timestamp - delay + ├── when eta is lower than the current block timestamp + delay │ └── it should revert with Timelock__ETAMustSatisfyDelay ├── when eta is greater than the current block timestamp + delay + GRACE_PERIOD │ └── it should revert with Timelock__ETAMustSatisfyDelay diff --git a/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo b/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo index 8b13789..9437b52 100644 --- a/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo +++ b/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo @@ -1 +1,166 @@ +use snforge_std::cheatcodes::events::EventFetcher; +use starknet::{ContractAddress, contract_address_const,}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock}; +use poseidon::PoseidonTrait; +use shisui::utils::hash::ISpanFelt252Hash; +use core::hash::HashStateTrait; +use core::hash::HashStateExTrait; +use snforge_std::{ + start_prank, stop_prank, start_warp, CheatTarget, spy_events, SpyOn, EventSpy, EventAssertions +}; +use array::ArrayTrait; +const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days + +// This test check queue transaction with caller isn't the admin address +// It calls timelock.queue_transaction() +// The test expects to fails with error Pending admin only +#[test] +#[should_panic(expected: ('Admin only',))] +fn when_caller_not_admin_it_should_revert() { + let (timelock, _) = init(); + + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 1; + + timelock.queue_transaction(target, signature, data.span(), eta); +} + +// This test check queue transaction with caller is the admin and eta is lower than current block timestamp + delay +// It calls timelock.queue_transaction() +// The test expects to fails with error ETA must satisfy delay +#[test] +#[should_panic(expected: ('ETA must satisfy delay',))] +fn when_caller_is_admin_and_eta_lower_than_block_and_delay_it_should_revert() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + // set pending admin + start_prank(CheatTarget::One(timelock_address), timelock_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock_address)); + // accept admin + start_prank(CheatTarget::One(timelock_address), admin_address); + timelock.accept_admin(); + //define block timestamp + start_warp(CheatTarget::One(timelock_address), 1000); + // define values + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = 1; + + timelock.queue_transaction(target, signature, data.span(), eta); +} + +// This test check queue transaction with caller is the admin and eta is greater than current block timestamp + delay + grace period +// It calls timelock.queue_transaction() +// The test expects to fails with error ETA must satisfy delay +#[test] +#[should_panic(expected: ('ETA must satisfy delay',))] +fn when_caller_is_admin_and_eta_greater_than_block_and_delay_and_grace_period_it_should_revert() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + // set pending admin + start_prank(CheatTarget::One(timelock_address), timelock_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock_address)); + // accept admin + start_prank(CheatTarget::One(timelock_address), admin_address); + timelock.accept_admin(); + //define block timestamp + start_warp(CheatTarget::One(timelock_address), 1000); + // define values + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = + 1383402; // value greater than 1000 (block timestamp)+172801 (delay) +1209600 (grace period) + timelock.queue_transaction(target, signature, data.span(), eta); +} + +// This test check queue transaction having tx_hash already in queue with caller is the admin and valid eta +// It calls timelock.queue_transaction() +// The test expects to fails with error Tx already queued +#[test] +#[should_panic(expected: ('Tx already queued',))] +fn when_caller_is_admin_and_valid_eta_and_tx_hash_already_in_queue_should_revert() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + // set pending admin + start_prank(CheatTarget::One(timelock_address), timelock_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock_address)); + // accept admin + start_prank(CheatTarget::One(timelock_address), admin_address); + timelock.accept_admin(); + //define block timestamp + start_warp(CheatTarget::One(timelock_address), 1000); + // define values + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 20000; + timelock.queue_transaction(target, signature, data.span(), eta); + timelock.queue_transaction(target, signature, data.span(), eta); +} + +// This test check queue transaction with caller is the admin and valid eta to work +// It calls timelock.queue_transaction() +// The test expects to succeed +#[test] +fn when_caller_is_admin_and_valid_eta_it_should_work() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + // set pending admin + start_prank(CheatTarget::One(timelock_address), timelock_address); + timelock.set_pending_admin(admin_address); + stop_prank(CheatTarget::One(timelock_address)); + // accept admin + start_prank(CheatTarget::One(timelock_address), admin_address); + timelock.accept_admin(); + //define block timestamp + start_warp(CheatTarget::One(timelock_address), 1000); + // define values + let target = contract_address_const::<'target'>(); + let signature = selector!("signature"); + let data = array!['0x01']; + let eta = MINIMUM_DELAY + 20000; + //generate hash + //let tx_hash = get_hash(target, signature, data.span(), eta); + let mut spy = spy_events(SpyOn::One(timelock_address)); + //call queue_transaction + timelock.queue_transaction(target, signature, data.span(), eta); + let tx_hash = 0x3d1394918f9a449d63ca74a79bdc7d573179ea887fae5a9896d1f93b6894384; + spy + .assert_emitted( + @array![ + ( + timelock_address, + Timelock::Event::QueueTransaction( + Timelock::QueueTransaction { + tx_hash, target, signature, input_data: data.span(), eta + } + ) + ) + ] + ); + assert(spy.events.len() == 0, 'There should be no events'); + assert(timelock.get_tx_status(tx_hash), 'Tx wrongly queued'); +} + +fn init() -> (ITimelockDispatcher, ContractAddress) { + tests::tests_lib::deploy_timelock_mock(MINIMUM_DELAY + 1, contract_address_const::<'admin'>()) +} + +fn get_hash( + _target: ContractAddress, _signature: felt252, _data: Span, _eta: u256 +) -> felt252 { + let mut hash_state = PoseidonTrait::new(); + hash_state = hash_state.update_with(_target); + hash_state = hash_state.update_with(_signature); + hash_state = hash_state.update_with(_data.hash_span()); + hash_state = hash_state.update_with(_eta); + hash_state.finalize() +} diff --git a/tests/integration/core/timelock/set_delay/test_set_delay.cairo b/tests/integration/core/timelock/set_delay/test_set_delay.cairo index 8b13789..88ca102 100644 --- a/tests/integration/core/timelock/set_delay/test_set_delay.cairo +++ b/tests/integration/core/timelock/set_delay/test_set_delay.cairo @@ -1 +1,67 @@ +use snforge_std::cheatcodes::events::EventFetcher; +use starknet::{ContractAddress, contract_address_const,}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock}; +use snforge_std::{ + start_prank, stop_prank, CheatTarget, spy_events, SpyOn, EventSpy, EventAssertions +}; +const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days +const MAXIMUM_DELAY: u256 = consteval_int!(15 * 24 * 60 * 60); // 15 days +const GRACE_PERIOD: u256 = consteval_int!(14 * 24 * 60 * 60); // 14 days + +// This testscheck set delay with value below minimum +// It calls timelock.set_delay() +// The test expects to fails with error Delay must exceed mininum delay +#[test] +#[should_panic(expected: ('Delay must exceed mininum delay',))] +fn given_delay_below_minimum_it_should_revert() { + let (timelock, _) = init(); + timelock.set_delay(MINIMUM_DELAY); +} + +// This test check set delay with value above maximum +// It calls timelock.set_delay() +// The test expects to fails with error Delay must under maximum delay +#[test] +#[should_panic(expected: ('Delay must under maximum delay',))] +fn given_delay_above_maximum_it_should_revert() { + let (timelock, _) = init(); + timelock.set_delay(MAXIMUM_DELAY); +} + +// This test check set delay with correct value but caller isn't timelock contract +// It calls timelock.set_delay() +// The test expects to fails with error Timelock only +#[test] +#[should_panic(expected: ('Timelock only',))] +fn given_valid_delay_and_caller_not_timelock_contract_it_should_revert() { + let (timelock, _) = init(); + timelock.set_delay(MAXIMUM_DELAY - 1); +} + +// This tests check set delay with correct value and caller = timelock contract +// It calls timelock.set_delay() +// The test expects to succeed +#[test] +fn given_valid_delay_and_caller_timelock_contract_it_should_set_delay_and_sent_event() { + let delay = MAXIMUM_DELAY - 1; + let (timelock, timelock_address) = init(); + start_prank(CheatTarget::One(timelock_address), timelock_address); + let mut spy = spy_events(SpyOn::One(timelock_address)); + timelock.set_delay(delay); + spy + .assert_emitted( + @array![ + ( + timelock_address, + Timelock::Event::NewDelay(Timelock::NewDelay { new_delay: delay }) + ) + ] + ); + assert(spy.events.len() == 0, 'There should be no events'); + assert(timelock.get_delay() == MAXIMUM_DELAY - 1, 'Wrong delay set') +} + +fn init() -> (ITimelockDispatcher, ContractAddress) { + tests::tests_lib::deploy_timelock_mock(MINIMUM_DELAY + 1, contract_address_const::<'admin'>()) +} diff --git a/tests/integration/core/timelock/set_pending_admin/test_set_pending_admin.cairo b/tests/integration/core/timelock/set_pending_admin/test_set_pending_admin.cairo index 8b13789..e5f0ab9 100644 --- a/tests/integration/core/timelock/set_pending_admin/test_set_pending_admin.cairo +++ b/tests/integration/core/timelock/set_pending_admin/test_set_pending_admin.cairo @@ -1 +1,49 @@ +use snforge_std::cheatcodes::events::EventFetcher; +use starknet::{ContractAddress, contract_address_const,}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock}; +use snforge_std::{ + start_prank, stop_prank, CheatTarget, spy_events, SpyOn, EventSpy, EventAssertions +}; +const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days + +// This test check set pending admin with caller isn't timelock contract +// It calls timelock.set_pending_admin() +// The test expects to fails with error Pending admin only +#[test] +#[should_panic(expected: ('Timelock only',))] +fn when_caller_not_timelock_contract_it_should_revert() { + let (timelock, _) = init(); + let admin_address = contract_address_const::<'admin'>(); + timelock.set_pending_admin(admin_address); +} + +// This test check set pending admin with caller = timelock contract +// It calls ttimelock.set_pending_admin() +// The test expects to succeed +#[test] +fn when_caller_is_timelock_contract_should_work() { + let (timelock, timelock_address) = init(); + let admin_address = contract_address_const::<'admin'>(); + start_prank(CheatTarget::One(timelock_address), timelock_address); + let mut spy = spy_events(SpyOn::One(timelock_address)); + timelock.set_pending_admin(admin_address); + spy + .assert_emitted( + @array![ + ( + timelock_address, + Timelock::Event::NewPendingAdmin( + Timelock::NewPendingAdmin { new_pending_admin: admin_address } + ) + ) + ] + ); + assert(spy.events.len() == 0, 'There should be no events'); + assert(admin_address == timelock.get_pending_admin(), 'Wrong pending admin set'); +} + + +fn init() -> (ITimelockDispatcher, ContractAddress) { + tests::tests_lib::deploy_timelock_mock(MINIMUM_DELAY + 1, contract_address_const::<'admin'>()) +} diff --git a/tests/tests_lib.cairo b/tests/tests_lib.cairo index 278db7f..8c9f158 100644 --- a/tests/tests_lib.cairo +++ b/tests/tests_lib.cairo @@ -1,8 +1,10 @@ +use core::traits::TryInto; use snforge_std::{ declare, start_prank, stop_prank, start_mock_call, stop_mock_call, ContractClassTrait, ContractClass, CheatTarget }; use starknet::{ContractAddress, Felt252TryIntoContractAddress, contract_address_const}; +use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait}; /// Utility function to pre-calculate the address of a mock contract & deploy it. @@ -15,7 +17,7 @@ use starknet::{ContractAddress, Felt252TryIntoContractAddress, contract_address_ /// # Returns /// /// * `ContractAddress` - The pre-calculated address of the deployed contract -fn deploy_mock_contract(contract: ContractClass, calldata: @Array) -> ContractAddress { +fn deploy_mock_contract_precalc_address(contract: ContractClass, calldata: @Array) -> ContractAddress { let future_deployed_address = contract.precalculate_address(calldata); start_prank(CheatTarget::One(future_deployed_address), contract_address_const::<'caller'>()); contract.deploy_at(calldata, future_deployed_address).unwrap() @@ -53,3 +55,44 @@ fn deploy_address_provider() -> ContractAddress { let constructor_calldata = array![]; deploy_mock_contract(contract, @constructor_calldata) } + +/// Utility function to deploy a simple storage mock contract and return its address. +/// +/// # Returns +/// +/// * `ContractAddress` - The address of the deployed data store contract. +fn deploy_simple_storage_mock() -> ContractAddress { + let contract = declare('SimpleStorage'); + deploy_mock_contract(contract, @array![]) +} + +/// Utility function to deploy timelock contract and return its address. +/// +/// # Returns +/// +/// * `ContractAddress` - The address of the deployed data store contract. +fn deploy_timelock_mock( + _delay: u256, _admin: ContractAddress +) -> (ITimelockDispatcher, ContractAddress) { + let contract = declare('Timelock'); + let constructor_calldata = array![_delay.low.into(), _delay.high.into(), _admin.into()]; + let timelock_address = deploy_mock_contract(contract, @constructor_calldata); + (ITimelockDispatcher { contract_address: timelock_address }, timelock_address) +} + + +/// Utility function to pre-calculate the address of a mock contract & deploy it. +/// +/// # Arguments +/// +/// * `contract` - The contract class +/// * `calldata` - The calldata used for the contract constructor +/// +/// # Returns +/// +/// * `ContractAddress` - The pre-calculated address of the deployed contract +fn deploy_mock_contract(contract: ContractClass, calldata: @Array) -> ContractAddress { + //let future_deployed_address = contract.precalculate_address(calldata); + //start_prank(CheatTarget::One(future_deployed_address), contract_address_const::<'caller'>()); + contract.deploy(calldata).unwrap() +} From 4a3751cb9e9915f0e639b8e12baa8c82f1b201ea Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Tue, 12 Dec 2023 22:17:15 +0100 Subject: [PATCH 2/7] implement timelock --- src/core/timelock.cairo | 38 ++++++++----------- src/mocks/erc20_mock.cairo | 4 +- src/utils/hash.cairo | 25 ++++++------ .../accept_admin/test_accept_admin.cairo | 15 +++++--- .../test_cancel_transaction.cairo | 2 +- .../test_execute_transaction.cairo | 10 ++--- .../test_queue_transaction.cairo | 12 +++--- tests/tests_lib.cairo | 4 +- 8 files changed, 53 insertions(+), 57 deletions(-) diff --git a/src/core/timelock.cairo b/src/core/timelock.cairo index fd589b2..aea3a9a 100644 --- a/src/core/timelock.cairo +++ b/src/core/timelock.cairo @@ -1,6 +1,5 @@ use starknet::ContractAddress; - #[starknet::interface] trait ITimelock { fn set_delay(ref self: TContractState, delay: u256); @@ -44,23 +43,14 @@ trait ITimelock { #[starknet::contract] mod Timelock { - use core::traits::Into; - use core::zeroable::Zeroable; - use core::array::ArrayTrait; - use core::starknet::event::EventEmitter; - use core::option::OptionTrait; - use core::serde::Serde; - use core::traits::TryInto; - use core::box::BoxTrait; + use core::hash::HashStateTrait; + use core::hash::HashStateExTrait; + use poseidon::PoseidonTrait; use starknet::{ ContractAddress, contract_address_const, get_caller_address, get_contract_address, - get_block_timestamp, call_contract_syscall + get_block_timestamp, call_contract_syscall, SyscallResultTrait }; - use poseidon::PoseidonTrait; use shisui::utils::hash::ISpanFelt252Hash; - use core::hash::HashStateTrait; - use core::hash::HashStateExTrait; - use starknet::SyscallResultTrait; const MINIMUM_DELAY: u256 = consteval_int!(2 * 24 * 60 * 60); // 2 days const MAXIMUM_DELAY: u256 = consteval_int!(15 * 24 * 60 * 60); // 15 days @@ -93,6 +83,7 @@ mod Timelock { #[derive(Drop, starknet::Event)] struct NewAdmin { + old_admin: ContractAddress, #[key] new_admin: ContractAddress } @@ -165,11 +156,11 @@ mod Timelock { // CONSTRUCTOR // ************************************************************************* #[constructor] - fn constructor(ref self: ContractState, _delay: u256, _admin: ContractAddress) { - self.is_valid_delay(_delay); - assert(_admin.is_non_zero(), Errors::Timelock__ZeroAddressAdmin); - self.admin.write(_admin); - self.delay.write(_delay); + fn constructor(ref self: ContractState, delay: u256, admin: ContractAddress) { + self.is_valid_delay(delay); + assert(admin.is_non_zero(), Errors::Timelock__ZeroAddressAdmin); + self.admin.write(admin); + self.delay.write(delay); } // ************************************************************************* @@ -217,9 +208,10 @@ mod Timelock { let caller = get_caller_address(); assert(caller == self.pending_admin.read(), Errors::Timelock__PendingAdminOnly); + let old_admin = self.admin.read(); self.admin.write(caller); self.pending_admin.write(contract_address_const::<0>()); - self.emit(NewAdmin { new_admin: caller }); + self.emit(NewAdmin { old_admin, new_admin: caller }); } fn set_pending_admin(ref self: ContractState, pending_admin: ContractAddress) { @@ -317,9 +309,9 @@ mod Timelock { // ************************************************************************* #[generate_trait] impl InternalFunctions of InternalFunctionsTrait { - fn is_valid_delay(self: @ContractState, _delay: u256) { - assert(_delay > MINIMUM_DELAY, Errors::Timelock__DelayMustExceedMininumDelay); - assert(_delay < MAXIMUM_DELAY, Errors::Timelock__DelayMustNotExceedMaximumDelay); + fn is_valid_delay(self: @ContractState, delay: u256) { + assert(delay > MINIMUM_DELAY, Errors::Timelock__DelayMustExceedMininumDelay); + assert(delay < MAXIMUM_DELAY, Errors::Timelock__DelayMustNotExceedMaximumDelay); } fn admin_only(self: @ContractState) { diff --git a/src/mocks/erc20_mock.cairo b/src/mocks/erc20_mock.cairo index 6f67664..512d3ea 100644 --- a/src/mocks/erc20_mock.cairo +++ b/src/mocks/erc20_mock.cairo @@ -44,7 +44,7 @@ mod ERC20Mock { #[constructor] fn constructor(ref self: ContractState, decimals: u8) { // Call the internal function that writes decimals to storage - self._set_decimals(decimals); + self.set_decimals(decimals); // Initialize ERC20 let name = 'ERC20Mock'; let symbol = 'MOCK'; @@ -75,7 +75,7 @@ mod ERC20Mock { // ************************************************************************* #[generate_trait] impl InternalImpl of InternalTrait { - fn _set_decimals(ref self: ContractState, decimals: u8) { + fn set_decimals(ref self: ContractState, decimals: u8) { self.decimals.write(decimals); } } diff --git a/src/utils/hash.cairo b/src/utils/hash.cairo index fb8f0a4..9a934f9 100644 --- a/src/utils/hash.cairo +++ b/src/utils/hash.cairo @@ -1,4 +1,7 @@ -use hash::{LegacyHash, HashStateTrait, HashStateExTrait}; +use poseidon::PoseidonTrait; +use core::hash::HashStateTrait; +use core::hash::HashStateExTrait; + trait ISpanFelt252Hash { fn hash_span(self: @T) -> felt252; @@ -7,19 +10,15 @@ trait ISpanFelt252Hash { impl HashSpanFelt252 of ISpanFelt252Hash> { fn hash_span(self: @Span) -> felt252 { - let mut call_data_state = LegacyHash::hash(0, *self); - call_data_state = LegacyHash::hash(call_data_state, (*self).len()); - call_data_state - } -} - -impl LegacyHashSpanFelt252 of LegacyHash> { - fn hash(mut state: felt252, mut value: Span) -> felt252 { + let mut hash_state = PoseidonTrait::new(); + let mut datas: Span = *self; loop { - match value.pop_front() { - Option::Some(item) => { state = LegacyHash::hash(state, *item); }, - Option::None(_) => { break state; }, + match datas.pop_front() { + Option::Some(item) => { hash_state = hash_state.update_with(*item); }, + Option::None(_) => { break; }, }; - } + }; + hash_state.finalize() } } + diff --git a/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo b/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo index cb90c00..67e6374 100644 --- a/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo +++ b/tests/integration/core/timelock/accept_admin/test_accept_admin.cairo @@ -24,13 +24,14 @@ fn when_caller_not_timelock_contract_it_should_revert() { #[test] fn when_caller_is_pending_admin_it_should_work() { let (timelock, timelock_address) = init(); - let admin_address = contract_address_const::<'admin'>(); + let old_admin_address = timelock.get_admin(); + let new_admin_address = contract_address_const::<'new_admin'>(); // set pending admin start_prank(CheatTarget::One(timelock_address), timelock_address); - timelock.set_pending_admin(admin_address); + timelock.set_pending_admin(new_admin_address); stop_prank(CheatTarget::One(timelock_address)); // accept admin - start_prank(CheatTarget::One(timelock_address), admin_address); + start_prank(CheatTarget::One(timelock_address), new_admin_address); let mut spy = spy_events(SpyOn::One(timelock_address)); timelock.accept_admin(); spy @@ -38,13 +39,17 @@ fn when_caller_is_pending_admin_it_should_work() { @array![ ( timelock_address, - Timelock::Event::NewAdmin(Timelock::NewAdmin { new_admin: admin_address }) + Timelock::Event::NewAdmin( + Timelock::NewAdmin { + old_admin: old_admin_address, new_admin: new_admin_address + } + ) ) ] ); assert(spy.events.len() == 0, 'There should be no events'); assert(timelock.get_pending_admin().is_zero(), 'Pending admin not reset'); - assert(timelock.get_admin() == admin_address, 'Wrong admin set'); + assert(timelock.get_admin() == new_admin_address, 'Wrong admin set'); } fn init() -> (ITimelockDispatcher, ContractAddress) { diff --git a/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo b/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo index 5833c20..442f217 100644 --- a/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo +++ b/tests/integration/core/timelock/cancel_transaction/test_cancel_transaction.cairo @@ -58,7 +58,7 @@ fn when_caller_is_admin_and_tx_in_queue_it_should_works() { let signature = selector!("signature"); let data = array!['0x01']; let eta = MINIMUM_DELAY + 20000; - let tx_hash = 0x3d1394918f9a449d63ca74a79bdc7d573179ea887fae5a9896d1f93b6894384; + let tx_hash = 0x594c81121ebd7aeb82dbd64afb237b821480ddf87e41d308940cda027cd7efd; timelock.queue_transaction(target, signature, data.span(), eta); let mut spy = spy_events(SpyOn::One(timelock_address)); timelock.cancel_transaction(target, signature, data.span(), eta); diff --git a/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo b/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo index 22831ed..a291813 100644 --- a/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo +++ b/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo @@ -175,12 +175,12 @@ fn queue_transaction( } fn get_hash( - _target: ContractAddress, _signature: felt252, _data: Span, _eta: u256 + target: ContractAddress, signature: felt252, data: Span, eta: u256 ) -> felt252 { let mut hash_state = PoseidonTrait::new(); - hash_state = hash_state.update_with(_target); - hash_state = hash_state.update_with(_signature); - hash_state = hash_state.update_with(_data.hash_span()); - hash_state = hash_state.update_with(_eta); + hash_state = hash_state.update_with(target); + hash_state = hash_state.update_with(signature); + hash_state = hash_state.update_with(data.hash_span()); + hash_state = hash_state.update_with(eta); hash_state.finalize() } diff --git a/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo b/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo index 9437b52..6fa9cc2 100644 --- a/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo +++ b/tests/integration/core/timelock/queue_transaction/test_queue_transaction.cairo @@ -132,7 +132,7 @@ fn when_caller_is_admin_and_valid_eta_it_should_work() { let mut spy = spy_events(SpyOn::One(timelock_address)); //call queue_transaction timelock.queue_transaction(target, signature, data.span(), eta); - let tx_hash = 0x3d1394918f9a449d63ca74a79bdc7d573179ea887fae5a9896d1f93b6894384; + let tx_hash = 0x594c81121ebd7aeb82dbd64afb237b821480ddf87e41d308940cda027cd7efd; spy .assert_emitted( @array![ @@ -155,12 +155,12 @@ fn init() -> (ITimelockDispatcher, ContractAddress) { } fn get_hash( - _target: ContractAddress, _signature: felt252, _data: Span, _eta: u256 + target: ContractAddress, signature: felt252, data: Span, eta: u256 ) -> felt252 { let mut hash_state = PoseidonTrait::new(); - hash_state = hash_state.update_with(_target); - hash_state = hash_state.update_with(_signature); - hash_state = hash_state.update_with(_data.hash_span()); - hash_state = hash_state.update_with(_eta); + hash_state = hash_state.update_with(target); + hash_state = hash_state.update_with(signature); + hash_state = hash_state.update_with(data.hash_span()); + hash_state = hash_state.update_with(eta); hash_state.finalize() } diff --git a/tests/tests_lib.cairo b/tests/tests_lib.cairo index 8c9f158..ec5fb57 100644 --- a/tests/tests_lib.cairo +++ b/tests/tests_lib.cairo @@ -72,10 +72,10 @@ fn deploy_simple_storage_mock() -> ContractAddress { /// /// * `ContractAddress` - The address of the deployed data store contract. fn deploy_timelock_mock( - _delay: u256, _admin: ContractAddress + delay: u256, admin: ContractAddress ) -> (ITimelockDispatcher, ContractAddress) { let contract = declare('Timelock'); - let constructor_calldata = array![_delay.low.into(), _delay.high.into(), _admin.into()]; + let constructor_calldata = array![delay.low.into(), delay.high.into(), admin.into()]; let timelock_address = deploy_mock_contract(contract, @constructor_calldata); (ITimelockDispatcher { contract_address: timelock_address }, timelock_address) } From e5da06081cddf50c04967e315f478985fe671dd5 Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Tue, 12 Dec 2023 22:30:45 +0100 Subject: [PATCH 3/7] implement timelock --- tests/tests_lib.cairo | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/tests_lib.cairo b/tests/tests_lib.cairo index ec5fb57..11699c7 100644 --- a/tests/tests_lib.cairo +++ b/tests/tests_lib.cairo @@ -17,7 +17,9 @@ use shisui::core::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait}; /// # Returns /// /// * `ContractAddress` - The pre-calculated address of the deployed contract -fn deploy_mock_contract_precalc_address(contract: ContractClass, calldata: @Array) -> ContractAddress { +fn deploy_mock_contract_precalc_address( + contract: ContractClass, calldata: @Array +) -> ContractAddress { let future_deployed_address = contract.precalculate_address(calldata); start_prank(CheatTarget::One(future_deployed_address), contract_address_const::<'caller'>()); contract.deploy_at(calldata, future_deployed_address).unwrap() From f6e699f5fd090b2a50ab6563849e7394404ed332 Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Tue, 19 Dec 2023 22:31:40 +0100 Subject: [PATCH 4/7] timelock --- src/core/timelock.cairo | 31 +++++++++---------- src/mocks/simple_contract_mock.cairo | 5 +-- .../test_execute_transaction.cairo | 8 +++-- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/core/timelock.cairo b/src/core/timelock.cairo index aea3a9a..0b86818 100644 --- a/src/core/timelock.cairo +++ b/src/core/timelock.cairo @@ -37,7 +37,7 @@ trait ITimelock { signature: felt252, data: Span, eta: u256, - ); + ) -> Span; } @@ -276,7 +276,7 @@ mod Timelock { signature: felt252, data: Span, eta: u256 - ) { + ) -> Span { self.admin_only(); let tx_hash = self.get_hash(target, signature, data, eta); @@ -286,21 +286,20 @@ mod Timelock { assert(get_block_timestamp().into() <= eta + GRACE_PERIOD, Errors::Timelock__TxExpired); self.queued_transactions.write(tx_hash, false); - match call_contract_syscall(target, signature, data) { - Result::Ok(return_data) => { - self - .emit( - ExecuteTransaction { - tx_hash, - target: target, - signature: signature, - input_data: data, - eta: eta - } - ); - }, - Result::Err(revert_reason) => { panic_with_felt252(Errors::Timelock__TxReverted); }, + let mut results: Array> = ArrayTrait::new(); + + let result = call_contract_syscall(target, signature, data); + if (result.is_err()) { + panic_with_felt252(Errors::Timelock__TxReverted); } + + self + .emit( + ExecuteTransaction { + tx_hash, target: target, signature: signature, input_data: data, eta: eta + } + ); + result.unwrap() } } diff --git a/src/mocks/simple_contract_mock.cairo b/src/mocks/simple_contract_mock.cairo index e5bde2c..572d568 100644 --- a/src/mocks/simple_contract_mock.cairo +++ b/src/mocks/simple_contract_mock.cairo @@ -1,6 +1,6 @@ #[starknet::interface] trait ISimpleStorage { - fn set(ref self: TContractState, x: u8); + fn set(ref self: TContractState, x: u8) -> u8; fn get(self: @TContractState) -> u8; } @@ -17,9 +17,10 @@ mod SimpleStorage { #[external(v0)] impl SimpleStorage of super::ISimpleStorage { - fn set(ref self: ContractState, x: u8) { + fn set(ref self: ContractState, x: u8) -> u8 { assert(x < 200, 'Invalid int, must be <= 200'); self.stored_data.write(x); + x } fn get(self: @ContractState) -> u8 { self.stored_data.read() diff --git a/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo b/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo index a291813..ac2e202 100644 --- a/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo +++ b/tests/integration/core/timelock/execute_transaction/test_execute_transaction.cairo @@ -108,15 +108,16 @@ fn when_caller_is_admin_and_block_timestamp_is_valid_and_call_not_valid_it_shoul fn when_caller_is_admin_and_block_timestamp_is_valid_and_call_is_valid_it_should_work() { let (timelock, simple_contract) = init(); set_admin(timelock); - + let value: felt252 = 4; let (signature, data, eta) = queue_transaction( - timelock, simple_contract.contract_address, 4, MINIMUM_DELAY + 20000 + timelock, simple_contract.contract_address, value, MINIMUM_DELAY + 20000 ); let tx_hash = get_hash(simple_contract.contract_address, signature, data, eta); start_warp(CheatTarget::One(timelock.contract_address), eta.try_into().unwrap() + 1); let mut spy = spy_events(SpyOn::One(timelock.contract_address)); - timelock.execute_transaction(simple_contract.contract_address, signature, data, eta); + let result: Span = timelock + .execute_transaction(simple_contract.contract_address, signature, data, eta); spy .assert_emitted( @array![ @@ -136,6 +137,7 @@ fn when_caller_is_admin_and_block_timestamp_is_valid_and_call_is_valid_it_should ); assert(simple_contract.get() == 4, 'Call to simple contract failed'); assert(!timelock.get_tx_status(tx_hash), 'Tx still in queued'); + assert(*result[0] == 4, 'Wrong value return by syscall'); } fn init() -> (ITimelockDispatcher, ISimpleStorageDispatcher) { From 7d000b6c4f9d532253467351454babde4b46da89 Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Tue, 19 Dec 2023 22:51:14 +0100 Subject: [PATCH 5/7] timelock --- src/core/timelock.cairo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/timelock.cairo b/src/core/timelock.cairo index 0b86818..e3e0878 100644 --- a/src/core/timelock.cairo +++ b/src/core/timelock.cairo @@ -286,7 +286,6 @@ mod Timelock { assert(get_block_timestamp().into() <= eta + GRACE_PERIOD, Errors::Timelock__TxExpired); self.queued_transactions.write(tx_hash, false); - let mut results: Array> = ArrayTrait::new(); let result = call_contract_syscall(target, signature, data); if (result.is_err()) { @@ -299,6 +298,7 @@ mod Timelock { tx_hash, target: target, signature: signature, input_data: data, eta: eta } ); + result.unwrap() } } From 88d92b226d390b834b1f1ab01c164399f6430a18 Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Thu, 4 Jan 2024 18:51:27 +0100 Subject: [PATCH 6/7] implement timelock --- src/core/timelock.cairo | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/timelock.cairo b/src/core/timelock.cairo index e3e0878..c1c9d99 100644 --- a/src/core/timelock.cairo +++ b/src/core/timelock.cairo @@ -83,6 +83,7 @@ mod Timelock { #[derive(Drop, starknet::Event)] struct NewAdmin { + #[key] old_admin: ContractAddress, #[key] new_admin: ContractAddress @@ -308,11 +309,13 @@ mod Timelock { // ************************************************************************* #[generate_trait] impl InternalFunctions of InternalFunctionsTrait { + #[inline(always)] fn is_valid_delay(self: @ContractState, delay: u256) { assert(delay > MINIMUM_DELAY, Errors::Timelock__DelayMustExceedMininumDelay); assert(delay < MAXIMUM_DELAY, Errors::Timelock__DelayMustNotExceedMaximumDelay); } + #[inline(always)] fn admin_only(self: @ContractState) { assert(get_caller_address() == self.admin.read(), Errors::Timelock__AdminOnly) } From 264a9b0208c6277dc026aeeb10721be5dc07dc7b Mon Sep 17 00:00:00 2001 From: 0xTitan Date: Thu, 4 Jan 2024 19:03:33 +0100 Subject: [PATCH 7/7] Implement Timelock --- src/core/timelock.cairo | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/timelock.cairo b/src/core/timelock.cairo index c1c9d99..555cf65 100644 --- a/src/core/timelock.cairo +++ b/src/core/timelock.cairo @@ -84,9 +84,8 @@ mod Timelock { #[derive(Drop, starknet::Event)] struct NewAdmin { #[key] + new_admin: ContractAddress, old_admin: ContractAddress, - #[key] - new_admin: ContractAddress } #[derive(Drop, starknet::Event)]