diff --git a/.github/workflows/build-lint-test.yml b/.github/workflows/build-lint-test.yml index 435d99d21..223d60b02 100644 --- a/.github/workflows/build-lint-test.yml +++ b/.github/workflows/build-lint-test.yml @@ -64,6 +64,7 @@ jobs: "lsp9init", "lsp11", "lsp11init", + "lsp17", "lsp20", "lsp20init", "lsp23", diff --git a/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol b/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol index 61b8206ea..6263eb509 100644 --- a/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol +++ b/contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol @@ -818,10 +818,11 @@ abstract contract LSP0ERC725AccountCore is if (success) { return result; } else { - // `result` -> first word in memory where the length of `result` is stored - // `add(result, 32)` -> next word in memory is where the `result` data starts + // `mload(result)` -> offset in memory where `result.length` is located + // `add(result, 32)` -> offset in memory where `result` data starts assembly { - revert(add(result, 32), mload(result)) + let resultdata_size := mload(result) + revert(add(result, 32), resultdata_size) } } } diff --git a/contracts/LSP17ContractExtension/LSP17Extendable.sol b/contracts/LSP17ContractExtension/LSP17Extendable.sol index c5461988b..c86c21b05 100644 --- a/contracts/LSP17ContractExtension/LSP17Extendable.sol +++ b/contracts/LSP17ContractExtension/LSP17Extendable.sol @@ -106,10 +106,13 @@ abstract contract LSP17Extendable is ERC165 { if (success) { return result; } else { - // `result` -> first word in memory where the length of `result` is stored - // `add(result, 32)` -> next word in memory is where the `result` data starts + // `mload(result)` -> offset in memory where `result.length` is located + // `add(result, 32)` -> offset in memory where `result` data starts + // solhint-disable no-inline-assembly + /// @solidity memory-safe-assembly assembly { - revert(add(result, 32), mload(result)) + let resultdata_size := mload(result) + revert(add(result, 32), resultdata_size) } } } diff --git a/contracts/LSP9Vault/LSP9VaultCore.sol b/contracts/LSP9Vault/LSP9VaultCore.sol index 0a41223e4..c60a10eec 100644 --- a/contracts/LSP9Vault/LSP9VaultCore.sol +++ b/contracts/LSP9Vault/LSP9VaultCore.sol @@ -169,10 +169,13 @@ contract LSP9VaultCore is if (success) { return result; } else { - // `result` -> first word in memory where the length of `result` is stored - // `add(result, 32)` -> next word in memory is where the `result` data starts + // `mload(result)` -> offset in memory where `result.length` is located + // `add(result, 32)` -> offset in memory where `result` data starts + // solhint-disable no-inline-assembly + /// @solidity memory-safe-assembly assembly { - revert(add(result, 32), mload(result)) + let resultdata_size := mload(result) + revert(add(result, 32), resultdata_size) } } } diff --git a/contracts/Mocks/FallbackExtensions/RevertErrorsTestExtension.sol b/contracts/Mocks/FallbackExtensions/RevertErrorsTestExtension.sol new file mode 100644 index 000000000..037fa7861 --- /dev/null +++ b/contracts/Mocks/FallbackExtensions/RevertErrorsTestExtension.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +/** + * @dev This contract is used only for testing purposes + */ +contract RevertErrorsTestExtension { + error SomeCustomError(address someAddress); + + function revertWithCustomError() public view { + revert SomeCustomError(msg.sender); + } + + function revertWithErrorString() public pure { + revert("some error message"); + } + + function revertWithPanicError() public pure { + uint256 number = 2; + + // trigger an arithmetic underflow. + // this should trigger a error of type `Panic(uint256)` + // with error code 17 (0x11) --> Panic(0x11) + number -= 10; + } + + function revertWithNoErrorData() public pure { + // solhint-disable reason-string + revert(); + } +} diff --git a/contracts/Mocks/LSP17ExtendableTester.sol b/contracts/Mocks/LSP17ExtendableTester.sol new file mode 100644 index 000000000..0cd60031a --- /dev/null +++ b/contracts/Mocks/LSP17ExtendableTester.sol @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.4; + +import {LSP17Extendable} from "../LSP17ContractExtension/LSP17Extendable.sol"; + +/** + * @dev This contract is used only for testing purposes + */ +contract LSP17ExtendableTester is LSP17Extendable { + mapping(bytes4 => address) internal _extensions; + + string internal _someStorageData; + string internal _anotherStorageData; + + // This `receive()` function is just put there to disable the following solc compiler warning: + // + // "This contract has a payable fallback function, but no receive ether function. + // Consider adding a receive ether function." + receive() external payable {} + + // solhint-disable no-complex-fallback + fallback() external payable { + // CHECK we can update the contract's storage BEFORE calling an extension + setStorageData("updated BEFORE calling `_fallbackLSP17Extendable`"); + + _fallbackLSP17Extendable(msg.data); + + // CHECK we can update the contract's storage AFTER calling an extension + setAnotherStorageData( + "updated AFTER calling `_fallbackLSP17Extendable`" + ); + } + + function getExtension( + bytes4 functionSelector + ) public view returns (address) { + return _getExtension(functionSelector); + } + + function setExtension( + bytes4 functionSelector, + address extensionContract + ) public { + _extensions[functionSelector] = extensionContract; + } + + function getStorageData() public view returns (string memory) { + return _someStorageData; + } + + function setStorageData(string memory newData) public { + _someStorageData = newData; + } + + function getAnotherStorageData() public view returns (string memory) { + return _anotherStorageData; + } + + function setAnotherStorageData(string memory newData) public { + _anotherStorageData = newData; + } + + function _getExtension( + bytes4 functionSelector + ) internal view override returns (address) { + return _extensions[functionSelector]; + } +} diff --git a/package.json b/package.json index 1f9451f64..cf0245c3d 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "test:lsp9init": "hardhat test --no-compile tests/LSP9Vault/LSP9VaultInit.test.ts", "test:lsp11": "hardhat test --no-compile tests/LSP11BasicSocialRecovery/LSP11BasicSocialRecovery.test.ts", "test:lsp11init": "hardhat test --no-compile tests/LSP11BasicSocialRecovery/LSP11BasicSocialRecoveryInit.test.ts", + "test:lsp17": "hardhat test --no-compile tests/LSP17ContractExtension/LSP17Extendable.test.ts", "test:lsp20": "hardhat test --no-compile tests/LSP20CallVerification/LSP6/LSP20WithLSP6.test.ts", "test:lsp20init": "hardhat test --no-compile tests/LSP20CallVerification/LSP6/LSP20WithLSP6Init.test.ts", "test:lsp23": "hardhat test --no-compile tests/LSP23LinkedContractsDeployment/LSP23LinkedContractsDeployment.test.ts", diff --git a/tests/LSP17ContractExtension/LSP17Extendable.behaviour.ts b/tests/LSP17ContractExtension/LSP17Extendable.behaviour.ts index 82d25e2de..411ded9ec 100644 --- a/tests/LSP17ContractExtension/LSP17Extendable.behaviour.ts +++ b/tests/LSP17ContractExtension/LSP17Extendable.behaviour.ts @@ -265,7 +265,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { + it('should revert with NoExtensionFoundForFunctionSelector, even if passed a different value from the msg.value', async () => { const sender = context.accounts[0]; const supposedValue = 200; const checkMsgVariableFunctionSignature = @@ -667,6 +667,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { await expect( context.accounts[0].sendTransaction({ @@ -677,7 +678,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { + describe('when calling with a payload prepended with 4 bytes of 0', () => { describe('when no extension is set for bytes4(0)', () => { describe('when the payload is `0x00000000`', () => { describe('with sending value', () => { @@ -694,6 +695,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { it('should pass', async () => { await expect( @@ -705,6 +707,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { describe('with sending value', () => { it('should pass and emit ValueReceived value', async () => { @@ -726,6 +729,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { it('should pass', async () => { const graffiti = @@ -744,6 +748,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { describe('when setting an extension that reverts', () => { let revertFallbackExtension: RevertFallbackExtension; @@ -762,6 +767,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { it('should revert', async () => { await expect( @@ -772,6 +778,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { it('should revert', async () => { const graffiti = @@ -801,7 +808,7 @@ export const shouldBehaveLikeLSP17 = (buildContext: () => Promise { + describe('when minting to the account', () => { describe('before setting the onERC721ReceivedExtension', () => { it('should fail since onERC721Received is not implemented', async () => { await expect(token.mint(context.contract.address)).to.be.reverted; diff --git a/tests/LSP17ContractExtension/LSP17Extendable.test.ts b/tests/LSP17ContractExtension/LSP17Extendable.test.ts new file mode 100644 index 000000000..130afe615 --- /dev/null +++ b/tests/LSP17ContractExtension/LSP17Extendable.test.ts @@ -0,0 +1,144 @@ +import { expect } from 'chai'; +import { ethers } from 'hardhat'; + +import { + LSP17ExtendableTester, + LSP17ExtendableTester__factory, + EmitEventExtension, + EmitEventExtension__factory, + RevertErrorsTestExtension, + RevertErrorsTestExtension__factory, +} from '../../types'; + +describe('LSP17Extendable - Basic Implementation', () => { + let accounts; + + let lsp17Implementation: LSP17ExtendableTester; + let exampleExtension: EmitEventExtension; + let errorsExtension: RevertErrorsTestExtension; + + const selectorWithExtension = + EmitEventExtension__factory.createInterface().getSighash('emitEvent'); + const selectorWithNoExtension = '0xdeadbeef'; + + // selectors to test that errors are bubbled up to the contract + const revertErrorsExtensionInterface = RevertErrorsTestExtension__factory.createInterface(); + + const selectorRevertCustomError = + revertErrorsExtensionInterface.getSighash('revertWithCustomError'); + + const selectorRevertErrorString = + revertErrorsExtensionInterface.getSighash('revertWithErrorString'); + + const selectorRevertPanicError = + revertErrorsExtensionInterface.getSighash('revertWithPanicError'); + + const selectorRevertNoErrorData = + revertErrorsExtensionInterface.getSighash('revertWithNoErrorData'); + + before('setup', async () => { + accounts = await ethers.getSigners(); + + lsp17Implementation = await new LSP17ExtendableTester__factory(accounts[0]).deploy(); + exampleExtension = await new EmitEventExtension__factory(accounts[0]).deploy(); + errorsExtension = await new RevertErrorsTestExtension__factory(accounts[0]).deploy(); + + await lsp17Implementation.setExtension(selectorWithExtension, exampleExtension.address); + + await lsp17Implementation.setExtension(selectorRevertCustomError, errorsExtension.address); + await lsp17Implementation.setExtension(selectorRevertErrorString, errorsExtension.address); + await lsp17Implementation.setExtension(selectorRevertPanicError, errorsExtension.address); + await lsp17Implementation.setExtension(selectorRevertNoErrorData, errorsExtension.address); + }); + + // make sure storage is cleared before running each test + afterEach(async () => { + await lsp17Implementation.setStorageData('0x'); + await lsp17Implementation.setAnotherStorageData('0x'); + }); + + describe('when there is no extension set', () => { + it('should revert with error `NoExtensionFoundForFunctionSelector', async () => { + await expect( + accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorWithNoExtension, + }), + ).to.be.revertedWithCustomError(lsp17Implementation, 'NoExtensionFoundForFunctionSelector'); + }); + }); + + describe('when there is an extension set', () => { + describe('if the extension does not revert', () => { + it('should pass and not revert', async () => { + await expect( + accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorWithExtension, + }), + ).to.emit(exampleExtension, 'EventEmittedInExtension'); + }); + + describe('if there is any code logic that run after extension was called', () => { + it("should have updated the contract's storage after the fallback LSP17 function ran", async () => { + const storageBefore = await lsp17Implementation.getStorageData(); + expect(storageBefore).to.equal('0x'); + + const anotherStorageBefore = await lsp17Implementation.getAnotherStorageData(); + expect(anotherStorageBefore).to.equal('0x'); + + await accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorWithExtension, + }); + + const storageAfter = await lsp17Implementation.getStorageData(); + expect(storageAfter).to.equal('updated BEFORE calling `_fallbackLSP17Extendable`'); + + const anotherStorageAfter = await lsp17Implementation.getAnotherStorageData(); + expect(anotherStorageAfter).to.equal('updated AFTER calling `_fallbackLSP17Extendable`'); + }); + }); + }); + + describe('if the extension revert', () => { + it('should bubble up custom errors', async () => { + await expect( + accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorRevertCustomError, + }), + ) + .to.be.revertedWithCustomError(errorsExtension, 'SomeCustomError') + .withArgs(lsp17Implementation.address); + }); + + it('should bubble up revert errors string', async () => { + await expect( + accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorRevertErrorString, + }), + ).to.be.revertedWith('some error message'); + }); + + it('should bubble up Panic type errors with their code', async () => { + await expect( + accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorRevertPanicError, + }), + ).to.be.revertedWithPanic('0x11' || 17); + }); + + it('should not bubble up anything with empty error data (`revert()`)', async () => { + await expect( + accounts[0].sendTransaction({ + to: lsp17Implementation.address, + data: selectorRevertNoErrorData, + }), + ).to.be.reverted; + }); + }); + }); +});