Itheum / datametaverse-evm

SDK to power Itheum's EVM web3 identity sub-system, SBT and DeSoc features.
GNU General Public License v3.0
1 stars 1 forks source link

How to prevent tokens (ETH, ERC20, NFT etc) being stored inside the identity smart contract and then getting "drained" by hacker #8

Open newbreedofgeek opened 2 years ago

newbreedofgeek commented 2 years ago

Imagine this scenario:

How can we solve this? Some ideas..

  1. ISM is configured to NOT all incoming storage/acceptance from any ERC20 or NFTs. The ONLY exception to this is that it can store Soulbound NFTs (ERC721NT)
  2. "proxy call" can ONLY be called via majority vote.. similar to how Gnosis Safe only executes a transfer of funds after majority agreement. This can be done internally to ISM like we do for "voting" out Owners

Best solution would be (1), where ISM Never holds any "funds" (tokens or ETH)... it's purely a proxy for the smart contract "receiver" to know that an "identity with claims reputation" is calling it and it can then make some decisions based on this.

Any ideas?

icegriffinguru commented 2 years ago

1 is the right choice IMO. It is because ISM represent my identity but it's not a wallet.

(I think we really need to reconsider Multi-Owner ISC plan. The more we build, the bigger burden it will make. Single-Owner ISC is better.)

newbreedofgeek commented 2 years ago

@0xpeho and I had a long discussion about this yesterday in our weekly sync.

(1) is a good option to stop user from using the identity contract as a wallet but unfortunately there does not seem to be a way to prevent this for ERC20 tokens as ERC20 can be sent to any contract address. We can prevent NFTs from being stored in the identity by enforcing ERC165 based whitelist for ONLY ERC721NT tokens to be sent (by adding checks here - https://github.com/Itheum/datametaverse-evm/blob/main/contracts/identity/Identity.sol#L146) and preventing all other NFTs. We cant also stop ETH from being sent, no do we want to as ETH is needed for proxy calls via ERC725X. Therefore, there is no guaranteed way to stop the identity from holding tokens.

So we are exploring (2), where proxy execute via ERC725X needs to be agreed upon by a majority owners. We may try and see if we can use gnosis-safe sdk or some other DAO sdk for this

0xpeho commented 2 years ago

Exactly. I also think the 2nd way is the one we should go to even if it's way harder to achieve. In the long run, an identity should be able to hold assets as well and if we don't restrict it to only allow ERC721NT it is way more beneficial. In the end, everything is a trade-off.