[SC] Unintended gas exhaustion attack #1445
Conversation
|
cc. @2dvorak, @hqjang-pepper, @nohkwak, @ian0371 |
| @@ -66,7 +66,7 @@ contract BridgeTransferERC721 is BridgeTokens, IERC721BridgeReceiver, BridgeTran | |||
| if (modeMintBurn) { | |||
| require(ERC721MetadataMintable(_tokenAddress).mintWithTokenURI(_to, _tokenId, _tokenURI), "mint failed"); | |||
| } else { | |||
| IERC721(_tokenAddress).safeTransferFrom(address(this), _to, _tokenId); | |||
| IERC721(_tokenAddress).transferFrom(address(this), _to, _tokenId); | |||
There was a problem hiding this comment.
If your change point is applied, onERC721Received() won't be executed even for a normal contract. So I'm concerned that there will be a side effect. Because if we simply use transferfrom(), then we can't check whether to address can receive token or not.
Is it intended?
There was a problem hiding this comment.
I've benn aware that's why the onERC721Received() stands for further treatment when a token receives. This further treatment is not available in the receiver address is not a token contract address. If the transaction signer is the sender of that transaction, we do not need to make this change. However, the problem of the current value transfer design is that the handle transaction of value transfer is signed by the operator on each chain. So, it seems inevitable change.
There was a problem hiding this comment.
Okay. There's another way to confirm whether we received ERC721 well or not,
But we never know which contract is malicious or not.
So this can be the proper solution
|
@hyunsooda, TODO: Doc update |
|
Added doc: klaytn/klaytn-docs#474 |
Proposed changes
As the handle of value transfer transaction is created by operators on parent/child chains, the contract call must not execute any other unintended code. The
safeTransferFrom()function callsonERC721Received()if the_toparameter is a contract address which can be attacker's contract. This change replacedsafeTransferFrom()withtransferFrom().Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...