EOS Knights Transfer Hack Statement

On September 14th around 3:00AM UTC EOSBET experienced a hack. Check this link for more detail.
https://medium.com/@eosbetcasino/eosbet-transfer-hack-statement-31a3be4f5dcf

Most contracts, including EOSBET, use similar logic. There was a similar code on our contract which posed the same risk. Fortunately, before the hacker attacked our account, prospectors.io reported the issue to us and evacuate our EOS to safe account.

The issue has been reported to us at 20:00 UTC on 14th. We fixed the risky code, then reported this issue to Korean dapp community. We will share some of this issue a little later to make time for other teams to check this issue.

Overview of the Attack

This is already mentioned in EOSBET, but I will explain it briefly here.

This is the code that can be attacked.

#undef EOSIO_ABI

#define EOSIO_ABI( TYPE, MEMBERS ) \
extern "C" { \
   void apply( uint64_t receiver, uint64_t code, uint64_t action ) { \
      if( action == N(onerror)) { \
         eosio_assert(code == N(eosio), "onerror action's are only valid from the \"eosio\" system account"); \
      } \
      auto self = receiver; \
      if( code == self || code == N(eosio.token) || action == N(onerror) ) { \
         TYPE thiscontract( self ); \
         switch( action ) { \
            EOSIO_API( TYPE, MEMBERS ) \
         } \
      } \
   } \
}

EOSIO_ABI(knights,  ....  (transfer) )

This is the way most of the contacts, including us, used to receive the transfer event of the eosio.token contact. The "code == N(eosio.token)" allows the event to be handled by the contact. And EOSIO_ABI exposes the transfer so that the request_recipient function of the eosio.token can invoke the transfer of our contact.

The problem is that the attacker could call the transfer action because the transfer action was exposed to the ABI. If this happens, contact thinks that event came after transfer. So contract run the code after deposit.

It's fixed code

#define EOSIO_ABI( TYPE, MEMBERS ) \
extern "C" { \
   void apply( uint64_t receiver, uint64_t code, uint64_t action ) { \
      auto self = receiver; \
      TYPE thiscontract( self ); \
      if( action == N(onerror)) { \
         eosio_assert(code == N(eosio), "onerror action's are only valid from the \"eosio\" system account"); \
      } \
      if( code == self ) { \
         if (action != N(transfer)) {\
            switch( action ) { \
                EOSIO_API( TYPE, MEMBERS ) \
            } \
         }\
      } \
      else if (code == N(eosio.token) && action == N(transfer) ) {\
          execute_action(&thiscontract, &knights::transfer);\
      }\
   } \

Transfer execution prevented with self scope. Transfer action is handled only as an event in the eosio.token.

A Note to the Community

Thank you for sharing the issue and for evacuating EOS safely. We have been able to avoid this issue without being hacked. If our code was attacked and no support from EOS community, it would take a long time to figure out the issue. I appreciate to community members for finding and sharing the issues.

EOS Knights intends to further strengthen its internal testing and code reviews to prevent such problems in the future. In addition, we will improve the contract so that there is not much EOS in the contract.

Thanks.