From 35ddbd585f7152a0ebe29d1e73e6b14f61eea255 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 10 Jan 2024 16:21:58 +0100 Subject: [PATCH] fix: restrict owner change in _beforeTokenTransfer --- .../LSP8Errors.sol | 5 ++ .../LSP8IdentifiableDigitalAssetCore.sol | 19 ++++--- .../Mocks/Tokens/LSP8TransferOwnerChange.sol | 51 +++++++++++++++++++ .../LSP8IdentifiableDigitalAsset.md | 31 +++++++++++ .../extensions/LSP8Burnable.md | 31 +++++++++++ .../extensions/LSP8CappedSupply.md | 31 +++++++++++ .../extensions/LSP8CompatibleERC721.md | 37 ++++++++++++-- .../extensions/LSP8Enumerable.md | 31 +++++++++++ .../presets/LSP8CompatibleERC721Mintable.md | 37 ++++++++++++-- .../presets/LSP8Mintable.md | 31 +++++++++++ .../LSP8Mintable.behaviour.ts | 39 ++++++++++++++ 11 files changed, 329 insertions(+), 14 deletions(-) create mode 100644 contracts/Mocks/Tokens/LSP8TransferOwnerChange.sol diff --git a/contracts/LSP8IdentifiableDigitalAsset/LSP8Errors.sol b/contracts/LSP8IdentifiableDigitalAsset/LSP8Errors.sol index 7bbf06ba0..1d603e932 100644 --- a/contracts/LSP8IdentifiableDigitalAsset/LSP8Errors.sol +++ b/contracts/LSP8IdentifiableDigitalAsset/LSP8Errors.sol @@ -103,3 +103,8 @@ error LSP8TokenIdsDataEmptyArray(); * @notice Batch call failed. */ error LSP8BatchCallFailed(uint256 callIndex); + +/** + * @dev Reverts when the token owner changed inside the {_beforeTokenTransfer} hook. + */ +error LSP8TokenOwnerChanged(bytes32 tokenId, address oldOwner, address newOwner); diff --git a/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAssetCore.sol b/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAssetCore.sol index 5b00006f8..085848c89 100644 --- a/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAssetCore.sol +++ b/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAssetCore.sol @@ -41,7 +41,8 @@ import { LSP8NotifyTokenReceiverIsEOA, LSP8TokenIdsDataLengthMismatch, LSP8TokenIdsDataEmptyArray, - LSP8BatchCallFailed + LSP8BatchCallFailed, + LSP8TokenOwnerChanged } from "./LSP8Errors.sol"; // constants @@ -634,9 +635,11 @@ abstract contract LSP8IdentifiableDigitalAssetCore is _beforeTokenTransfer(from, to, tokenId, data); - // Re-fetch and update `tokenOwner` in case `tokenId` - // was transferred inside the `_beforeTokenTransfer` hook - tokenOwner = tokenOwnerOf(tokenId); + // Check that `tokenId`'s owner was not changed inside the `_beforeTokenTransfer` hook + address currentTokenOwner = tokenOwnerOf(tokenId); + if (tokenOwner != currentTokenOwner) { + revert LSP8TokenOwnerChanged(tokenId, tokenOwner, currentTokenOwner); + } _clearOperators(from, tokenId); @@ -721,8 +724,8 @@ abstract contract LSP8IdentifiableDigitalAssetCore is * @dev Attempt to notify the operator `operator` about the `tokenId` being authorized. * This is done by calling its {universalReceiver} function with the `_TYPEID_LSP8_TOKENOPERATOR` as typeId, if `operator` is a contract that supports the LSP1 interface. * If `operator` is an EOA or a contract that does not support the LSP1 interface, nothing will happen and no notification will be sent. - - * @param operator The address to call the {universalReceiver} function on. + + * @param operator The address to call the {universalReceiver} function on. * @param lsp1Data the data to be sent to the `operator` address in the `universalReceiver` call. */ function _notifyTokenOperator( @@ -740,8 +743,8 @@ abstract contract LSP8IdentifiableDigitalAssetCore is * @dev Attempt to notify the token sender `from` about the `tokenId` being transferred. * This is done by calling its {universalReceiver} function with the `_TYPEID_LSP8_TOKENSSENDER` as typeId, if `from` is a contract that supports the LSP1 interface. * If `from` is an EOA or a contract that does not support the LSP1 interface, nothing will happen and no notification will be sent. - - * @param from The address to call the {universalReceiver} function on. + + * @param from The address to call the {universalReceiver} function on. * @param lsp1Data the data to be sent to the `from` address in the `universalReceiver` call. */ function _notifyTokenSender( diff --git a/contracts/Mocks/Tokens/LSP8TransferOwnerChange.sol b/contracts/Mocks/Tokens/LSP8TransferOwnerChange.sol new file mode 100644 index 000000000..3f595f65a --- /dev/null +++ b/contracts/Mocks/Tokens/LSP8TransferOwnerChange.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: Apache-2.0 + +pragma solidity ^0.8.4; + +// modules +import { + LSP8IdentifiableDigitalAsset +} from "../../LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAsset.sol"; +import { + LSP8Burnable +} from "../../LSP8IdentifiableDigitalAsset/extensions/LSP8Burnable.sol"; + +contract LSP8TransferOwnerChange is LSP8IdentifiableDigitalAsset, LSP8Burnable { + // solhint-disable-next-line no-empty-blocks + constructor( + string memory name_, + string memory symbol_, + address newOwner_, + uint256 lsp4TokenType_, + uint256 lsp8TokenIdFormat_ + ) + LSP8IdentifiableDigitalAsset( + name_, + symbol_, + newOwner_, + lsp4TokenType_, + lsp8TokenIdFormat_ + ) + {} + + function mint( + address to, + bytes32 tokenId, + bool force, + bytes memory data + ) public { + _mint(to, tokenId, force, data); + } + + function _beforeTokenTransfer( + address, + address, + bytes32 tokenId, + bytes memory + ) internal override { + // if tokenID exist transfer token ownership to this contract + if (_exists(tokenId)) { + _tokenOwners[tokenId] = address(this); + } + } +} diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAsset.md b/docs/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAsset.md index f58cf9036..708d6a325 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAsset.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAsset.md @@ -2066,6 +2066,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8IdentifiableDigitalAsset.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAsset.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Burnable.md b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Burnable.md index b6ac5fb1b..56f4230c9 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Burnable.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Burnable.md @@ -2092,6 +2092,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8Burnable.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Burnable.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CappedSupply.md b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CappedSupply.md index cd7d4f6bd..372210480 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CappedSupply.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CappedSupply.md @@ -2108,6 +2108,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8CappedSupply.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CappedSupply.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.md b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.md index 885463cc7..f6ddd11d0 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.md @@ -1677,7 +1677,7 @@ Approve `operator` to operate on all tokens of `tokensOwner`. event Approval(address indexed owner, address indexed approved, uint256 indexed tokenId); ``` -Emitted when the allowance of a `spender` for an `owner` is set by a call to [`approve`](#approve). `value` is the new allowance. +Emitted when `owner` enables `approved` to manage the `tokenId` token. #### Parameters @@ -1704,7 +1704,7 @@ Emitted when the allowance of a `spender` for an `owner` is set by a call to [`a event ApprovalForAll(address indexed owner, address indexed operator, bool approved); ``` -Emitted when `account` grants or revokes permission to `operator` to transfer their tokens, according to `approved`. +Emitted when `owner` enables or disables (`approved`) `operator` to manage all of its assets. #### Parameters @@ -1897,7 +1897,7 @@ Emitted when `tokenId` token is transferred from the `from` to the `to` address. event Transfer(address indexed from, address indexed to, uint256 indexed tokenId); ``` -Emitted when `value` tokens are moved from one account (`from`) to another (`to`). Note that `value` may be zero. +Emitted when `tokenId` token is transferred from `from` to `to`. #### Parameters @@ -2457,6 +2457,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8CompatibleERC721.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8CompatibleERC721.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Enumerable.md b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Enumerable.md index f04d2e4b6..9495edf7f 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Enumerable.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Enumerable.md @@ -2094,6 +2094,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8Enumerable.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/extensions/LSP8Enumerable.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8CompatibleERC721Mintable.md b/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8CompatibleERC721Mintable.md index 01b0bfd1f..46f2ebdb6 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8CompatibleERC721Mintable.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8CompatibleERC721Mintable.md @@ -1721,7 +1721,7 @@ Approve `operator` to operate on all tokens of `tokensOwner`. event Approval(address indexed owner, address indexed approved, uint256 indexed tokenId); ``` -Emitted when the allowance of a `spender` for an `owner` is set by a call to [`approve`](#approve). `value` is the new allowance. +Emitted when `owner` enables `approved` to manage the `tokenId` token. #### Parameters @@ -1748,7 +1748,7 @@ Emitted when the allowance of a `spender` for an `owner` is set by a call to [`a event ApprovalForAll(address indexed owner, address indexed operator, bool approved); ``` -Emitted when `account` grants or revokes permission to `operator` to transfer their tokens, according to `approved`. +Emitted when `owner` enables or disables (`approved`) `operator` to manage all of its assets. #### Parameters @@ -1941,7 +1941,7 @@ Emitted when `tokenId` token is transferred from the `from` to the `to` address. event Transfer(address indexed from, address indexed to, uint256 indexed tokenId); ``` -Emitted when `value` tokens are moved from one account (`from`) to another (`to`). Note that `value` may be zero. +Emitted when `tokenId` token is transferred from `from` to `to`. #### Parameters @@ -2526,6 +2526,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8CompatibleERC721Mintable.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8CompatibleERC721Mintable.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8Mintable.md b/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8Mintable.md index e5d4d8dd9..1a1c1ee76 100644 --- a/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8Mintable.md +++ b/docs/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8Mintable.md @@ -2157,6 +2157,37 @@ Reverts when trying to authorize or revoke the token's owner as an operator.
+### LSP8TokenOwnerChanged + +:::note References + +- Specification details: [**LSP-8-IdentifiableDigitalAsset**](https://github.com/lukso-network/lips/tree/main/LSPs/LSP-8-IdentifiableDigitalAsset.md#lsp8tokenownerchanged) +- Solidity implementation: [`LSP8Mintable.sol`](https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/LSP8IdentifiableDigitalAsset/presets/LSP8Mintable.sol) +- Error signature: `LSP8TokenOwnerChanged(bytes32,address,address)` +- Error hash: `0x5a9c31d3` + +::: + +```solidity +error LSP8TokenOwnerChanged( + bytes32 tokenId, + address oldOwner, + address newOwner +); +``` + +Reverts when the token owner changed inside the [`_beforeTokenTransfer`](#_beforetokentransfer) hook. + +#### Parameters + +| Name | Type | Description | +| ---------- | :-------: | ----------- | +| `tokenId` | `bytes32` | - | +| `oldOwner` | `address` | - | +| `newOwner` | `address` | - | + +
+ ### NoExtensionFoundForFunctionSelector :::note References diff --git a/tests/LSP8IdentifiableDigitalAsset/LSP8Mintable.behaviour.ts b/tests/LSP8IdentifiableDigitalAsset/LSP8Mintable.behaviour.ts index e5061c0d4..b2045352e 100644 --- a/tests/LSP8IdentifiableDigitalAsset/LSP8Mintable.behaviour.ts +++ b/tests/LSP8IdentifiableDigitalAsset/LSP8Mintable.behaviour.ts @@ -3,6 +3,7 @@ import { ethers } from 'hardhat'; import { expect } from 'chai'; import { LSP8Mintable, + LSP8TransferOwnerChange, UniversalProfile, LSP6KeyManager, UniversalReceiverDelegateTokenReentrant__factory, @@ -166,4 +167,42 @@ export const shouldBehaveLikeLSP8Mintable = ( expect(tokenIdsOfUP[1]).to.equal(ethers.utils.hexlify(secondRandomTokenId)); }); }); + describe('when there is an owner change in the _beforeTokenTransfer hook', () => { + it('should revert', async () => { + // deploy LSP8TransferOwnerChange contract + const LSP8TransferOwnerChange = await ethers.getContractFactory('LSP8TransferOwnerChange'); + const lsp8TransferOwnerChange = (await LSP8TransferOwnerChange.deploy( + 'RandomName', + 'RandomSymbol', + context.accounts.owner.address, + 0, // token type + 0, // token id format + )) as LSP8TransferOwnerChange; + + const randomTokenId = ethers.utils.randomBytes(32); + + // // mint a token tokenReceiver + await lsp8TransferOwnerChange.connect(context.accounts.owner).mint( + context.accounts.tokenReceiver.address, + randomTokenId, + true, // beneficiary is an EOA, so we need to force minting + '0x', + ); + + // transfer ownership to lsp8TransferOwnerChange + await expect( + lsp8TransferOwnerChange + .connect(context.accounts.tokenReceiver) + .transfer( + context.accounts.tokenReceiver.address, + context.accounts.owner.address, + randomTokenId, + true, + '0x', + ), + ) + .to.be.revertedWithCustomError(lsp8TransferOwnerChange, 'LSP8TokenOwnerChanged') + .withArgs(randomTokenId, context.accounts.tokenReceiver.address, lsp8TransferOwnerChange.address); + }); + }); };