exo-digital-labs / ERC721R

https://erc721r.super.site/
MIT License
240 stars 60 forks source link

Fix: Owner Not allow to call the refund #11

Closed lawweiliang closed 2 years ago

lawweiliang commented 2 years ago

Hi @elie222

Here is the improvements.

  1. Stop the owner of the contract from calling the refund function

    • Create a Modifier function called notOwner and being call over refund function.
    • Testing code was added as well over test file.
  2. To simplify contributions, I did include a prettier config file.

    • If other developers want to contribute, they need not to change their prettier setting over their vs code.

    Hopes it helps.

    Regards, Liang

qbig commented 2 years ago

Correct me if I am wrong. @lawweiliang this is still vulnerable cuz owner can be transfer to someone else? https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

f0rmatting commented 2 years ago

Hi @elie222 @lawweiliang Maybe is not enough, "refundAddress" can changed by setRefundAddress function, we should be prohibited all refundAddress call refund function.

elie222 commented 2 years ago

Hi @elie222 @lawweiliang Maybe is not enough, "refundAddress" can changed by setRefundAddress function, we should be prohibited all refundAddress call refund function.

But owner can still transfer to another address that isn't blacklisted. I left the same comment on your PR. We just merged in another fix for this issue: https://github.com/exo-digital-labs/ERC721R/pull/9. Happy to hear everyone's thoughts on it.

f0rmatting commented 2 years ago

issue: https://github.com/exo-digital-labs/ERC721R/pull/9 is better.

lawweiliang commented 2 years ago

@qbig, you are right, owner still can transfer the token to other and refund it using the new address and continue to drain the fund. Hmm...

lawweiliang commented 2 years ago

Refund problem was solved over #9 by adding has refunded. The token transfer problem by the owner was solved as well. I closed this pull request.