diff --git a/contracts/pToken.sol b/contracts/pToken.sol index 4eccca1..1e49c5e 100644 --- a/contracts/pToken.sol +++ b/contracts/pToken.sol @@ -15,6 +15,8 @@ contract PToken is bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); bytes4 public ORIGIN_CHAIN_ID; + event ReceiveUserDataFailed(); + event Redeem( address indexed redeemer, uint256 value, @@ -78,13 +80,15 @@ contract PToken is ); _mint(recipient, value, userData, operatorData); if (userData.length > 0) { - try IPReceiver(recipient).receiveUserData(userData) {} catch { - // The recipient may be not implementing the IPReceiver interface, - // or the receiveUserData() hook may have a bug inside that would - // revert the entire transaction, thus blocking the minting process. - // Swallowing the error here permits pNetwork to fulfill its role - // of relaying messages/tokens across chains. - } + // pNetwork aims to deliver cross chain messages successfully regardless of what the user may do with them. + // We do not want this mint transaction reverting if their receiveUserData function reverts, + // and thus we swallow any such errors, emitting a HookFailed event instead. + // The low-level call is used because in the solidity version this contract was written in, + // a try/catch block fails to catch the revert caused if the receiver is not in fact a contract. + // This way, a user also has the option include userData even when minting to an externally owned account. + bytes memory data = abi.encodeWithSelector(IPReceiver.receiveUserData.selector, userData); + (bool success, ) = recipient.call(data); + if (!success) emit ReceiveUserDataFailed(); } return true; } diff --git a/contracts/pTokenNoGSN.sol b/contracts/pTokenNoGSN.sol index 10441af..be56f42 100644 --- a/contracts/pTokenNoGSN.sol +++ b/contracts/pTokenNoGSN.sol @@ -19,6 +19,8 @@ contract PTokenNoGSN is bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); bytes4 public ORIGIN_CHAIN_ID; + event ReceiveUserDataFailed(); + event Redeem( address indexed redeemer, uint256 value, @@ -81,13 +83,15 @@ contract PTokenNoGSN is ); _mint(recipient, value, userData, operatorData); if (userData.length > 0) { - try IPReceiver(recipient).receiveUserData(userData) {} catch { - // The recipient may be not implementing the IPReceiver interface, - // or the receiveUserData() hook may have a bug inside that would - // revert the entire transaction, thus blocking the minting process. - // Swallowing the error here permits pNetwork to fulfill its role - // of relaying messages/tokens across chains. - } + // pNetwork aims to deliver cross chain messages successfully regardless of what the user may do with them. + // We do not want this mint transaction reverting if their receiveUserData function reverts, + // and thus we swallow any such errors, emitting a HookFailed event instead. + // The low-level call is used because in the solidity version this contract was written in, + // a try/catch block fails to catch the revert caused if the receiver is not in fact a contract. + // This way, a user also has the option include userData even when minting to an externally owned account. + bytes memory data = abi.encodeWithSelector(IPReceiver.receiveUserData.selector, userData); + (bool success, ) = recipient.call(data); + if (!success) emit ReceiveUserDataFailed(); } return true; } diff --git a/contracts/test-contracts/PReceiver.sol b/contracts/test-contracts/PReceiver.sol index dbfa640..6aaf33a 100644 --- a/contracts/test-contracts/PReceiver.sol +++ b/contracts/test-contracts/PReceiver.sol @@ -9,3 +9,12 @@ contract PReceiver is IPReceiver { emit UserData(userData); } } + +contract PReceiverReverting is IPReceiver { + function receiveUserData(bytes calldata) external override { + require(false, "Revert!"); + } +} + +contract NotImplementing { +} diff --git a/test/05-ptoken.test.js b/test/05-ptoken.test.js index 84e87a2..e191f95 100644 --- a/test/05-ptoken.test.js +++ b/test/05-ptoken.test.js @@ -145,21 +145,24 @@ USE_GSN.map(_useGSN => } }) - it('`mint()` w/ data should revert if receiver is not a contract', async () => { + it('`mint()` w/ data should mint tokens to a non-contract address & emit correct events', async () => { const data = '0xdead' + const expectedNumEvents = 2 const operatorData = '0xb33f' const recipient = OTHERS[0].address - try { - const tx = await CONTRACT['mint(address,uint256,bytes,bytes)'](recipient, AMOUNT, data, operatorData) - await tx.wait() - assert.fail('Should not succeed!') - } catch (_err) { - const expectedErr = 'Transaction reverted: function call to a non-contract account' - assert(_err.message.includes(expectedErr)) - } + const recipientBalanceBefore = await getTokenBalance(recipient, CONTRACT) + const tx = + await CONTRACT['mint(address,uint256,bytes,bytes)'](recipient, AMOUNT, data, operatorData) + const { events } = await tx.wait() + const recipientBalanceAfter = await getTokenBalance(recipient, CONTRACT) + assert(recipientBalanceBefore.eq(BigNumber.from(0))) + assert(recipientBalanceAfter.eq(BigNumber.from(AMOUNT))) + assert.strictEqual(events.length, expectedNumEvents) + assertTransferEvent(events, ZERO_ADDRESS, recipient, AMOUNT) + assertMintEvent(events, recipient, OWNER.address, AMOUNT, data, operatorData) }) - it('`mint()` w/ data should mint tokens & emit correct events', async () => { + it('`mint()` w/ data should mint & call receiveUserData & emit correct events', async () => { const data = '0xdead' const expectedNumEvents = 3 const operatorData = '0xb33f' @@ -178,9 +181,56 @@ USE_GSN.map(_useGSN => assertTransferEvent(events, ZERO_ADDRESS, recipientContract.address, AMOUNT) assertMintEvent(events, recipientContract.address, OWNER.address, AMOUNT, data, operatorData) const userDataEvent = recipientContract.interface.parseLog(events.at(-1)) + assert.strictEqual(userDataEvent.name, 'UserData') assert.strictEqual(userDataEvent.args.data, data) }) + // eslint-disable-next-line max-len + it('`mint()` w/ `userData` should mint tokens, emit correct events & not revert despite `receiveUserData` hook being not implemented', + async () => { + const data = '0xdead' + const expectedNumEvents = 3 + const operatorData = '0xb33f' + const recipientContract = await ethers + .getContractFactory('contracts/test-contracts/PReceiver.sol:NotImplementing') + .then(_factory => upgrades.deployProxy(_factory)) + + const recipientBalanceBefore = await getTokenBalance(recipientContract.address, CONTRACT) + const tx = + await CONTRACT['mint(address,uint256,bytes,bytes)'](recipientContract.address, AMOUNT, data, operatorData) + const { events } = await tx.wait() + const recipientBalanceAfter = await getTokenBalance(recipientContract.address, CONTRACT) + assert(recipientBalanceBefore.eq(BigNumber.from(0))) + assert(recipientBalanceAfter.eq(BigNumber.from(AMOUNT))) + assert.strictEqual(events.length, expectedNumEvents) + assertTransferEvent(events, ZERO_ADDRESS, recipientContract.address, AMOUNT) + assertMintEvent(events, recipientContract.address, OWNER.address, AMOUNT, data, operatorData) + assert.strictEqual(events.at(-1).event, 'ReceiveUserDataFailed') + }) + + // eslint-disable-next-line max-len + it('`mint()` w/ `userData` should mint tokens, emit correct events & not revert despite `receiveUserData` hook reverting', + async () => { + const data = '0xdead' + const expectedNumEvents = 3 + const operatorData = '0xb33f' + const recipientContract = await ethers + .getContractFactory('contracts/test-contracts/PReceiver.sol:PReceiverReverting') + .then(_factory => upgrades.deployProxy(_factory)) + + const recipientBalanceBefore = await getTokenBalance(recipientContract.address, CONTRACT) + const tx = + await CONTRACT['mint(address,uint256,bytes,bytes)'](recipientContract.address, AMOUNT, data, operatorData) + const { events } = await tx.wait() + const recipientBalanceAfter = await getTokenBalance(recipientContract.address, CONTRACT) + assert(recipientBalanceBefore.eq(BigNumber.from(0))) + assert(recipientBalanceAfter.eq(BigNumber.from(AMOUNT))) + assert.strictEqual(events.length, expectedNumEvents) + assertTransferEvent(events, ZERO_ADDRESS, recipientContract.address, AMOUNT) + assertMintEvent(events, recipientContract.address, OWNER.address, AMOUNT, data, operatorData) + assert.strictEqual(events.at(-1).event, 'ReceiveUserDataFailed') + }) + it('Should revert when minting tokens with the contract address as the recipient', async () => { const recipient = CONTRACT.address try {