Dev #1
Dev #1
Conversation
contracts/AccessNFT.sol
Outdated
| @@ -16,7 +16,7 @@ import { ProtocolControl } from "./ProtocolControl.sol"; | |||
| // Royalties | |||
| import "@openzeppelin/contracts/interfaces/IERC2981.sol"; | |||
|
|
|||
| contract AccessNFT is ERC1155PresetMinterPauser, IERC1155Receiver, ERC2771Context, IERC2981 { | |||
| contract AccessNFT is ERC1155PresetMinterPauser, ERC2771Context, IERC2981 { | |||
There was a problem hiding this comment.
AccessNFT needs to be IERC1155Receiver since the contract initially holds the access NFTs (NFTs of the 'redeemed' state) whenever createAccessNfts of createAccessPack is called.
https://github.com/nftlabs/nftlabs-protocols/blob/dev/contracts/AccessNFT.sol#L159
| @@ -46,6 +46,23 @@ contract Coin is ERC20PresetMinterPauser, ERC2771Context { | |||
| _contractURI = _uri; | |||
| } | |||
|
|
|||
| function _beforeTokenTransfer( | |||
There was a problem hiding this comment.
If a smart contract calls transferFrom, where both from and to have the TRANSFER_ROLE -- the transfer will be successful even if the caller smart contract (labeled as the "operator" in e.g. ERC 1155) does not have the TRANSFER_ROLE.
Is this behaviour intended?
| function _isApprovedOrOwner(address spender, uint256 tokenId) internal view override returns (bool) { | ||
|
|
||
| if(controlCenter.isRestrictedTransfers(address(this))) { | ||
| function _beforeTokenTransfer( |
There was a problem hiding this comment.
For ERC 721, the TRANSFER_ROLE check cannot happen in the _beforeTokenTransfer hook. The reason is that an operator e.g. an approved marketplace contract can initiate the transfer from from to to.
If we care about the initiator of a transfer having the TRANSFER_ROLE, we must make the check in the _isApprovedOrOwner hook - this is also a hook that runs on every transfer, and lets you check whether the operator, not just from and to, has the TRANSFER_ROLE
contracts/AccessNFT.sol
Outdated
| @@ -256,7 +256,8 @@ contract AccessNFT is ERC1155PresetMinterPauser, IERC1155Receiver, ERC2771Contex | |||
|
|
|||
| if(controlCenter.isRestrictedTransfers(address(this))) { | |||
| require( | |||
| controlCenter.hasRole(controlCenter.TRANSFER_ROLE(), operator), | |||
| controlCenter.hasRole(controlCenter.TRANSFER_ROLE(), from) || | |||
There was a problem hiding this comment.
We must make a check for the operator. As far as I understood, we want the initiator of the transfer to have the TRANSFER_ROLE - the initiator of a transfer is always the operator, but not always from.
If I myself am transferring tokens to you, then my address is both the operator and from. But if some other address I've approved to be able to move my tokens, that other address is the operator and I'm from. If we don't make the check for operator, the operator can initiate transfers (whenever from has approved it) even if the operator does not have TRANSFER_ROLE
* NFT Stake optimizations (#417) * Update Struct `Staker` * uint256 amountStake => uint64 amountStaked * uint256 conditionIdOflastUpdate => uint64 conditionIdOflastUpdate * uint256 timeOfLastUpdate => uint128 timeOfLastUpdate * Other associated changes to facilitate use of updated struct * Remove `stakersArray` and reorder varibles `stakersArray` does not appear to serve any purpose other than to allow for easy off-chain retreival of all staked users. Moving the declaration of indexArray below the non-dynamic variables allows for more efficient variable packing. This saves ~2K gas on the staking function. * Remove ownership and approval check from `_stake` The removed checks are included in all modern ERC721 implementations. If a project removes them from their ERC721 contract, they should override the `_stake` function and include them there. However, in most instances, it is redundant to include them in `_stake`. * linting * run yarn gas * update gas report to ensure accuracy * Re-add stakers array * yarn gas --------- Co-authored-by: WhiteOakKong <92236155+WhiteOakKong@users.noreply.github.com>
No description provided.