search

ditto-lab / ditto

NFT Future protocol
MIT License
60 stars 3 forks source link

use `encodeCall` to save on EXTCODESIZE when a call to contract is guaranteed #76

Closed 0xbok closed 2 years ago

0xbok commented 2 years ago

should be happening only on nft transfers: https://github.com/ditto-lab/ditto/blob/7f50ae20bc8c874da737f64e81eba60af4745bfe/src/DittoMachine.sol#L445-L448

0xbok commented 2 years ago

fixed here: https://github.com/ditto-lab/ditto/pull/74/files#diff-5f403a7772c17d7680ce01535fa3091f9e4e609225525650553439abb1b46d0dL446-L449

removed the ownership/balance check as the nft transfers will fail if that's the case. We use SafeTransferLib for erc20 tokens which doesn't do EXTCODESIZE check.

calvbore commented 2 years ago

I'm a little concerned that removing those checks will make it easier for a malicious token to attack ditto. For example, if it's an upgradeable token that functions as expected but is then upgraded so that the functions do not execute the intended effects. I'm not sure if this is a realistic cause for concern yet?

0xbok commented 2 years ago

the PR I linked removes the ownership and balance check, so extcodesize is still done. After this change, I dont think we should remove the code size check.

As a general note, my view is that the attack surface of a malicious token is limited to the clones obtained through that token, and also the final nft trade that happens for that token. There can be many other ways a malicious token can hurt the users. Like an upgradeable token which later changes its implementation to have onlyOwner functions to change balances.

So I believe we should only aim to accommodate genuine ERC20 tokens.