Audit: Revert dispatch if default fee not matched

contracts/src/contracts/mailbox.cairo

@@ -118,12 +118,15 @@ pub mod mailbox {
         pub const UNEXPECTED_DESTINATION: felt252 = 'Unexpected destination';
         pub const ALREADY_DELIVERED: felt252 = 'Mailbox: already delivered';
         pub const ISM_VERIFICATION_FAILED: felt252 = 'Mailbox:ism verification failed';
+        pub const ISM_CANNOT_BE_NULL: felt252 = 'ISM cannot be null';
+        pub const OWNER_CANNOT_BE_NULL: felt252 = 'ISM cannot be null';
+        pub const HOOK_CANNOT_BE_NULL: felt252 = 'Hook cannot be null';
         pub const NO_ISM_FOUND: felt252 = 'ISM: no ISM found';
         pub const NEW_OWNER_IS_ZERO: felt252 = 'Ownable: new owner cannot be 0';
         pub const ALREADY_OWNER: felt252 = 'Ownable: already owner';
         pub const INSUFFICIENT_BALANCE: felt252 = 'Insufficient balance';
         pub const INSUFFICIENT_ALLOWANCE: felt252 = 'Insufficient allowance';
-        pub const NOT_ENOUGH_FEE_PROVIDED: felt252 = 'Provided fee < required fee';
+        pub const NOT_ENOUGH_FEE_PROVIDED: felt252 = 'Provided fee < needed fee';
     }
 
     #[constructor]
@@ -135,6 +138,10 @@ pub mod mailbox {
         _default_hook: ContractAddress,
         _required_hook: ContractAddress
     ) {
+        assert(_default_ism != contract_address_const::<0>(), Errors::ISM_CANNOT_BE_NULL);
+        assert(_default_hook != contract_address_const::<0>(), Errors::HOOK_CANNOT_BE_NULL);
+        assert(_default_hook != contract_address_const::<0>(), Errors::HOOK_CANNOT_BE_NULL);
+        assert(owner != contract_address_const::<0>(), Errors::OWNER_CANNOT_BE_NULL);
         self.local_domain.write(_local_domain);
         self.default_ism.write(_default_ism);
         self.default_hook.write(_default_hook);
@@ -266,43 +273,41 @@ pub mod mailbox {
 
             // HOOKS
 
-            let caller_address = get_caller_address();
             let required_hook_address = self.required_hook.read();
             let required_hook = IPostDispatchHookDispatcher {
                 contract_address: required_hook_address
             };
-            let token_dispatcher = ERC20ABIDispatcher { contract_address: ETH_ADDRESS() };
-
             let mut required_fee = required_hook
                 .quote_dispatch(hook_metadata.clone(), message.clone());
-            if (required_fee > 0) {
-                assert(_fee_amount >= required_fee, Errors::NOT_ENOUGH_FEE_PROVIDED);
-                let contract_address = get_contract_address();
-                let user_balance = token_dispatcher.balanceOf(caller_address);
-                assert(user_balance >= _fee_amount, Errors::INSUFFICIENT_BALANCE);
-                assert(
-                    token_dispatcher.allowance(caller_address, contract_address) >= _fee_amount,
-                    Errors::INSUFFICIENT_ALLOWANCE
-                );
 
+            let hook_dispatcher = IPostDispatchHookDispatcher { contract_address: hook };
+            let default_fee = hook_dispatcher
+                .quote_dispatch(hook_metadata.clone(), message.clone());
+
+            assert(_fee_amount >= required_fee + default_fee, Errors::NOT_ENOUGH_FEE_PROVIDED);
+
+            let caller_address = get_caller_address();
+            let contract_address = get_contract_address();
+
+            let token_dispatcher = ERC20ABIDispatcher { contract_address: ETH_ADDRESS() };
+            let user_balance = token_dispatcher.balanceOf(caller_address);
+
+            assert(user_balance >= required_fee + default_fee, Errors::INSUFFICIENT_BALANCE);
+
+            assert(
+                token_dispatcher.allowance(caller_address, contract_address) >= _fee_amount,
+                Errors::INSUFFICIENT_ALLOWANCE
+            );
+
+            if (required_fee > 0) {
                 token_dispatcher.transferFrom(caller_address, required_hook_address, required_fee);
             }
-
             required_hook.post_dispatch(hook_metadata.clone(), message.clone(), required_fee);
 
-            if (hook != contract_address_const::<0>()) {
-                let hook_dispatcher = IPostDispatchHookDispatcher { contract_address: hook };
-                let default_fee = hook_dispatcher
-                    .quote_dispatch(hook_metadata.clone(), message.clone());
-                if (default_fee == 0) {
-                    hook_dispatcher
-                        .post_dispatch(hook_metadata.clone(), message.clone(), default_fee);
-                }
-                if (_fee_amount - required_fee >= default_fee) {
-                    token_dispatcher.transferFrom(caller_address, hook, default_fee);
-                    hook_dispatcher.post_dispatch(hook_metadata, message.clone(), default_fee);
-                }
+            if (default_fee > 0) {
+                token_dispatcher.transferFrom(caller_address, hook, default_fee);
             }
+            hook_dispatcher.post_dispatch(hook_metadata, message.clone(), default_fee);
 
             id
         }

contracts/src/tests/test_mailbox.cairo

@@ -302,7 +302,74 @@ fn test_dispatch_with_two_fee_hook() {
 }
 
 #[test]
-#[should_panic(expected: ('Provided fee < required fee',))]
+#[should_panic(expected: ('Provided fee < needed fee',))]
+fn test_dispatch_with_two_fee_hook_fails_if_greater_than_required_and_lower_than_default() {
+    let (_, protocol_fee_hook) = setup_protocol_fee();
+    let mock_hook = setup_mock_fee_hook();
+    let (mailbox, mut spy, _, _) = setup_mailbox(
+        MAILBOX(),
+        Option::Some(protocol_fee_hook.contract_address),
+        Option::Some(mock_hook.contract_address)
+    );
+    let erc20_dispatcher = ERC20ABIDispatcher { contract_address: ETH_ADDRESS() };
+    let ownable = IOwnableDispatcher { contract_address: ETH_ADDRESS() };
+    start_prank(CheatTarget::One(ownable.contract_address), OWNER());
+    // (mock_fee_hook consummes 3 * PROTOCOL_FEE)
+    erc20_dispatcher.approve(MAILBOX(), 3 * PROTOCOL_FEE);
+    stop_prank(CheatTarget::One(ownable.contract_address));
+    // The owner has the initial fee token supply
+    let ownable = IOwnableDispatcher { contract_address: mailbox.contract_address };
+    start_prank(CheatTarget::One(ownable.contract_address), OWNER());
+    let array = array![
+        0x01020304050607080910111213141516,
+        0x01020304050607080910111213141516,
+        0x01020304050607080910000000000000
+    ];
+
+    let message_body = BytesTrait::new(42, array);
+    let message = Message {
+        version: HYPERLANE_VERSION,
+        nonce: 0,
+        origin: LOCAL_DOMAIN,
+        sender: OWNER(),
+        destination: DESTINATION_DOMAIN,
+        recipient: RECIPIENT_ADDRESS(),
+        body: message_body.clone()
+    };
+    let (message_id, _) = MessageTrait::format_message(message.clone());
+    mailbox
+        .dispatch(
+            DESTINATION_DOMAIN,
+            RECIPIENT_ADDRESS(),
+            message_body,
+            3 * PROTOCOL_FEE,
+            Option::None,
+            Option::None
+        );
+    let expected_event = mailbox::Event::Dispatch(
+        mailbox::Dispatch {
+            sender: OWNER(),
+            destination_domain: DESTINATION_DOMAIN,
+            recipient_address: RECIPIENT_ADDRESS(),
+            message: message
+        }
+    );
+    let expected_event_id = mailbox::Event::DispatchId(mailbox::DispatchId { id: message_id });
+
+    spy
+        .assert_emitted(
+            @array![
+                (mailbox.contract_address, expected_event),
+                (mailbox.contract_address, expected_event_id)
+            ]
+        );
+
+    // balance check
+    assert_eq!(erc20_dispatcher.balance_of(OWNER()), INITIAL_SUPPLY - 4 * PROTOCOL_FEE);
+    assert(mailbox.get_latest_dispatched_id() == message_id, 'Failed to fetch latest id');
+}
+#[test]
+#[should_panic(expected: ('Provided fee < needed fee',))]
 fn test_dispatch_with_protocol_fee_hook_fails_if_provided_fee_lower_than_required_fee() {
     let (_, protocol_fee_hook) = setup_protocol_fee();
     let mock_hook = setup_mock_hook();

contracts/src/tests/test_validator_announce.cairo

@@ -69,8 +69,7 @@ fn test_double_announce() {
         180946006308525359965345158532346553211983108462325076142963585023296502126,
         276191619276790668637754154763775604
     ];
-    let res = validator_announce
-        .announce(validator_address, _storage_location_2.clone(), signature);
+    validator_announce.announce(validator_address, _storage_location_2.clone(), signature);
     let validators = validator_announce.get_announced_validators();
     assert(validators == array![validator_address].span(), 'validator array mismatch');
     let storage_location = validator_announce.get_announced_storage_locations(validators);