block oversized authority in account recovery request (HF29)

Author: ABW

libraries/chain/hive_evaluator_account.cpp

@@ -497,17 +497,21 @@ void request_account_recovery_evaluator::do_apply( const request_account_recover
   const auto& recovery_request_idx = _db.get_index< account_recovery_request_index, by_account >();
   auto request = recovery_request_idx.find( o.account_to_recover );
 
-  FC_TODO( "validate_auth_size should always be called" );
-    //ABW: now there is a bug - editing existing request can introduce authority wider than max,
-    //such authority can also be used when cancelling (although it is kind of "workaroundable")
-    //HF29 is needed to fix this, after that the check should be moved to validate
+  bool checked_size = false;
+  // TODO: after activation of HF29 check if the check can be moved to operation validate
+  if( _db.is_in_control() || _db.has_hardfork( HIVE_HARDFORK_1_29_FIX_ENFORCEMENT_OF_AUTHORITY_SIZE_ON_RECOVERY_REQUEST ) )
+  {
+    validate_auth_size( o.new_owner_authority );
+    checked_size = true;
+  }
 
   if( request == recovery_request_idx.end() ) // New Request
   {
     HIVE_CHAIN_STATE_ASSERT( !o.new_owner_authority.is_impossible() && "Cannot recover using an impossible authority.", o.account_to_recover, "Recovery authority for '${subject}' is impossible." );
     HIVE_CHAIN_STATE_ASSERT( o.new_owner_authority.weight_threshold, o.account_to_recover, "Cannot recover using an open authority." );
 
-    validate_auth_size( o.new_owner_authority );
+    if( !checked_size )
+      validate_auth_size( o.new_owner_authority );
 
     // Check accounts in the new authority exist
     verify_authority_accounts_exist( _db, o.new_owner_authority, o.account_to_recover, authority::owner );

libraries/protocol/hardfork.d/1_29.hf

@@ -20,4 +20,6 @@
 
 // allow modification of existing recurrent transfer with executions=1 (issue #786)
 #define HIVE_HARDFORK_1_29_ALLOW_MODIFY_LAST_RECURRENT_TRANSFER (HIVE_HARDFORK_1_29)
+// authority size limit can no longer be circumvented with use of account recovery
+#define HIVE_HARDFORK_1_29_FIX_ENFORCEMENT_OF_AUTHORITY_SIZE_ON_RECOVERY_REQUEST (HIVE_HARDFORK_1_29)
 #endif

tests/unit/db_fixture/database_fixture.cpp

@@ -143,7 +143,7 @@ void database_fixture::generate_block(uint32_t skip, const fc::ecc::private_key&
 
 void database_fixture::generate_block_from_pending()
 {
-  db_plugin->debug_generate_blocks( fc::optional< fc::ecc::private_key >(), 1, default_skip, 0, true, true );
+  db_plugin->debug_generate_blocks( committee, 1, default_skip, 0, true, true );
 }
 
 uint32_t database_fixture::generate_blocks(const fc::ecc::private_key& debug_key, uint32_t count, uint32_t skip)

tests/unit/tests/hf29_tests.cpp

@@ -0,0 +1,130 @@
+#ifdef IS_TEST_NET
+
+#include <boost/test/unit_test.hpp>
+
+#include <hive/chain/database_exceptions.hpp>
+#include <hive/chain/detail/state/account_object.hpp>
+#include <hive/chain/detail/state/account_object_multiindex.hpp>
+
+#include <hive/plugins/debug_node/debug_node_plugin.hpp>
+
+#include "../db_fixture/clean_database_fixture.hpp"
+
+using namespace hive::chain;
+using namespace hive::protocol;
+
+BOOST_FIXTURE_TEST_SUITE( hf29_tests, cluster_database_fixture )
+
+BOOST_AUTO_TEST_CASE( recovery_request_oversized_authority )
+{
+  try
+  {
+    bool is_hf29 = false;
+
+    auto _content = [ &is_hf29 ]( ptr_hardfork_database_fixture& executor )
+    {
+      BOOST_TEST_MESSAGE( "Testing: oversized authority in recovery request edit/cancel" );
+      BOOST_REQUIRE_EQUAL( (bool)executor, true );
+
+      ACTORS_EXT( (*executor), (alice)(bob) )
+      executor->fund( "alice", HIVE_asset( 1'000'000 ) );
+      executor->db_plugin->debug_update( []( database& db )
+      {
+        const auto& alice = db.get_account( "alice" );
+        db.modify( db.get_account( "bob" ), [&]( account_object& a )
+        {
+          a.set_recovery_account( alice );
+        } );
+      } );
+      executor->generate_block();
+
+      request_account_recovery_operation initial_request;
+      initial_request.recovery_account = "alice";
+      initial_request.account_to_recover = "bob";
+      initial_request.new_owner_authority = authority(
+        1, executor->generate_private_key( "k0" ).get_public_key(), 1 );
+      executor->push_transaction( initial_request, alice_private_key );
+      executor->generate_block();
+
+      const auto& idx = executor->db->get_index< account_recovery_request_index, by_account >();
+      BOOST_REQUIRE( idx.find( "bob" ) != idx.end() );
+      BOOST_REQUIRE_EQUAL( idx.find( "bob" )->get_new_owner_authority().key_auths.size(), 1u );
+
+      authority oversized;
+      oversized.weight_threshold = 1;
+      for( int i = 0; i <= HIVE_MAX_AUTHORITY_MEMBERSHIP; ++i )
+        oversized.add_authority( executor->generate_private_key( "extra_" + std::to_string( i ) ).get_public_key(), 1 );
+
+      request_account_recovery_operation oversized_edit;
+      oversized_edit.recovery_account = "alice";
+      oversized_edit.account_to_recover = "bob";
+      oversized_edit.new_owner_authority = oversized;
+
+      // regular push_transaction is stopped on check due to "is_in_control"
+      HIVE_REQUIRE_ASSERT( executor->push_transaction( oversized_edit, alice_private_key ),
+        "size <= HIVE_MAX_AUTHORITY_MEMBERSHIP" );
+
+      // work around "is_in_control" by not actually executing transaction
+      executor->db_plugin->debug_push_pending_transaction(
+        executor->build_transaction( oversized_edit, alice_private_key ) );
+
+      if( is_hf29 )
+      {
+        // "p2p" block is stopped on faulty transaction after HF29
+        HIVE_REQUIRE_ASSERT( executor->generate_block_from_pending(),
+          "size <= HIVE_MAX_AUTHORITY_MEMBERSHIP" );
+      }
+      else
+      {
+        // "p2p" block is ok before HF28, even with faulty transaction
+        executor->generate_block_from_pending();
+        // oversized authority is included in edited request
+        BOOST_REQUIRE_EQUAL( idx.find( "bob" )->get_new_owner_authority().key_auths.size(),
+          size_t( HIVE_MAX_AUTHORITY_MEMBERSHIP + 1 ) );
+      }
+
+      // weight_threshold == 0 marks cancellation
+      oversized.weight_threshold = 0;
+
+      request_account_recovery_operation oversized_cancel;
+      oversized_cancel.recovery_account = "alice";
+      oversized_cancel.account_to_recover = "bob";
+      oversized_cancel.new_owner_authority = oversized;
+
+      HIVE_REQUIRE_ASSERT( executor->push_transaction( oversized_cancel, alice_private_key ),
+        "size <= HIVE_MAX_AUTHORITY_MEMBERSHIP" );
+
+      executor->db->clear_pending();
+      executor->db_plugin->debug_push_pending_transaction(
+        executor->build_transaction( oversized_cancel, alice_private_key ) );
+
+      if( is_hf29 )
+      {
+        HIVE_REQUIRE_ASSERT( executor->generate_block_from_pending(),
+          "size <= HIVE_MAX_AUTHORITY_MEMBERSHIP" );
+        // request not cancelled - still in the index with the original small authority
+        BOOST_REQUIRE( idx.find( "bob" ) != idx.end() );
+        BOOST_REQUIRE_EQUAL( idx.find( "bob" )->get_new_owner_authority().key_auths.size(), 1u );
+      }
+      else
+      {
+        executor->generate_block_from_pending();
+        // oversized cancellation went through - request removed
+        BOOST_REQUIRE( idx.find( "bob" ) == idx.end() );
+      }
+    };
+
+    BOOST_TEST_MESSAGE( "*****HF-28*****" );
+    execute_hardfork<28>( _content );
+
+    is_hf29 = true;
+
+    BOOST_TEST_MESSAGE( "*****HF-29*****" );
+    execute_hardfork<29>( _content );
+  }
+  FC_LOG_AND_RETHROW()
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+
+#endif