search

eluv-io / contracts

Eluvio Content Management Smart Contracts
MIT License
10 stars 0 forks source link

Fix null signer - check signer not null in all token signature functions #95

Closed elv-serban closed 2 years ago

elv-serban commented 2 years ago

All signature checks allow for a null signature - the ecrecover returns 0 and the openzeppelin _isApprovedForOwner() allows a spender that is address(0) becuase getApproved(token) id returns 0.

The fix is to specifically require sender/signer/spender is not 0

function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
        address owner = ERC721.ownerOf(tokenId);
        return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
    }
elv-serban commented 2 years ago

Why not put the require check in _isApprovedOrOwner?

That is part of the openzeppelin library. I checked the latest version and it is still the same. I will make a ticket on that and will link here. It's strange they don't even have a mention on this in the comments

elv-peter commented 2 years ago

In that case a helper wrapper function can avoid the copy/paste

elv-serban commented 2 years ago

In that case a helper wrapper function can avoid the copy/paste

Agreed! Will do in another change set. But I would like to hear form openzeppelin first

elv-serban commented 2 years ago

https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3659