chore(protocol-contracts): check for null address when withdrawing (#1282)

* chore(protocol-contracts): check for null address when withdrawing

* chore(protocol-contracts): fix typo

Author: jatZama

protocol-contracts/governance/contracts/GovernanceOAppSender.sol

@@ -22,6 +22,8 @@ contract GovernanceOAppSender is OAppSender, OAppOptionsType3 {
     error FailedToWithdrawETH();
     /// @notice Thrown when trying to send cross-chain tx if contract holds unsufficient ETH to pay the LZ fees.
     error InsufficientBalanceForFee();
+    /// @notice Thrown when recipient is the null address.
+    error InvalidNullRecipient();
     /// @notice Thrown when trying to deploy this contract on an unsupported blockchain.
     error UnsupportedChainID();
 
@@ -167,8 +169,10 @@ contract GovernanceOAppSender is OAppSender, OAppOptionsType3 {
 
     /// @dev Allows the owner to withdraw ETH held by the contract.
     /// @param amount Amount of withdrawn ETH.
-    /// @param recipient Receiver of the withdrawn ETH.
+    /// @param recipient Receiver of the withdrawn ETH, should be non-null.
     function withdrawETH(uint256 amount, address recipient) external onlyOwner {
+        if (recipient == address(0)) revert InvalidNullRecipient();
+
         (bool success, ) = recipient.call{ value: amount }("");
         if (!success) {
             revert FailedToWithdrawETH();

protocol-contracts/governance/test/GovernanceOApp.test.ts

@@ -2,7 +2,7 @@ import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
 import { expect } from 'chai'
 import { Contract, ContractFactory } from 'ethers'
 import { deployments, ethers } from 'hardhat'
-import { execTransaction } from './utils/execTransaction.ts'
+import { execTransaction } from './utils/execTransaction'
 
 import { Options } from '@layerzerolabs/lz-v2-utilities'
 import { EndpointId } from '@layerzerolabs/lz-definitions'
@@ -259,6 +259,25 @@ describe('GovernanceOApp Test', function () {
         expect(diff <= tolerance).to.equal(true)
     })
 
+    it('should not send recovered funds to null address', async function () {
+        await owner.sendTransaction({
+            to: governanceOAppSender.address,
+            value: ethers.utils.parseEther('1'),
+        })
+        const tx = governanceOAppSender
+            .connect(owner)
+            .withdrawETH(ethers.utils.parseEther('1'), ethers.constants.AddressZero)
+        try {
+            await tx
+            expect.fail('withdrawETH should have reverted with InvalidNullRecipient')
+        } catch (err: any) {
+            const data = err.data
+            const selector = data.slice(0, 10)
+            const expected = governanceOAppSender.interface.getSighash('InvalidNullRecipient()')
+            expect(selector).to.equal(expected)
+        }
+    })
+
     it('should send a payable remote proposal', async function () {
         expect(BigInt(await gatewayConfigMock.value())).to.equal(0n)
         await owner.sendTransaction({

protocol-contracts/token/contracts/ZamaERC20.sol

@@ -21,8 +21,9 @@ contract ZamaERC20 is ERC20, ERC20Permit, ERC1363, ERC20Burnable, AccessControl,
     event ERC20Recovered(address indexed token, address indexed recipient, uint256 amount);
     event ERC721Recovered(address indexed token, uint256 tokenId, address indexed recipient);
 
-    error FailedToSendEther();
     error AmountsReceiversLengthMismatch();
+    error FailedToSendEther();
+    error InvalidNullRecipient();
 
     /**
      * @param name Name of the token.
@@ -79,10 +80,11 @@ contract ZamaERC20 is ERC20, ERC20Permit, ERC1363, ERC20Burnable, AccessControl,
     /**
      * @dev Allows the sender to recover Ether held by the contract.
      * @param amount Amount of recovered ETH.
-     * @param recipient Receiver of the recovered ETH.
+     * @param recipient Receiver of the recovered ETH, should be non-null.
      * @dev Emits an EtherRecovered event upon success.
      */
     function recoverEther(uint256 amount, address recipient) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        if (recipient == address(0)) revert InvalidNullRecipient();
         (bool success, ) = recipient.call{ value: amount }("");
         if (!success) {
             revert FailedToSendEther();
@@ -94,10 +96,11 @@ contract ZamaERC20 is ERC20, ERC20Permit, ERC1363, ERC20Burnable, AccessControl,
      * @dev Allows the sender to recover ERC20 tokens held by the contract.
      * @param token The address of the ERC20 token to recover.
      * @param amount The amount of the ERC20 token to recover.
-     * @param recipient Receiver of the recovered tokens.
+     * @param recipient Receiver of the recovered tokens, should be non-null.
      * @dev Emits an ERC20Recovered event upon success.
      */
     function recoverERC20(address token, uint256 amount, address recipient) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        if (recipient == address(0)) revert InvalidNullRecipient();
         IERC20(token).safeTransfer(recipient, amount);
         emit ERC20Recovered(token, recipient, amount);
     }
@@ -106,10 +109,11 @@ contract ZamaERC20 is ERC20, ERC20Permit, ERC1363, ERC20Burnable, AccessControl,
      * @dev Allows the sender to recover ERC721 tokens held by the contract.
      * @param token The address of the ERC721 token to recover.
      * @param tokenId The token ID of the ERC721 token to recover.
-     * @param recipient Receiver of the recovered ERC721 token.
+     * @param recipient Receiver of the recovered ERC721 token, should be non-null.
      * @dev Emits an ERC721Recovered event upon success.
      */
     function recoverERC721(address token, uint256 tokenId, address recipient) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        if (recipient == address(0)) revert InvalidNullRecipient();
         IERC721(token).safeTransferFrom(address(this), recipient, tokenId);
         emit ERC721Recovered(token, tokenId, recipient);
     }

protocol-contracts/token/test/ZamaERC20.test.ts

@@ -679,6 +679,12 @@ describe('ZamaERC20 - Unit Test', () => {
                         expect(etherAliceBalanceAfter).to.eq(etherAliceBalanceBefore.add(SEND_AMOUNT))
                     })
 
+                    it('should not send recovered ETH to null address', async () => {
+                        await expect(
+                            zamaERC20.connect(admin).recoverEther(SEND_AMOUNT, ethers.constants.AddressZero)
+                        ).to.be.revertedWithCustomError(zamaERC20, 'InvalidNullRecipient')
+                    })
+
                     it('should fail when trying to recover more than available balance', async () => {
                         await expect(
                             zamaERC20.connect(admin).recoverEther(SEND_AMOUNT.mul(2), alice.address)
@@ -732,6 +738,14 @@ describe('ZamaERC20 - Unit Test', () => {
                         expect(mockERC20AliceBalanceAfter).to.eq(mockERC20AliceBalanceBefore.add(SEND_AMOUNT))
                     })
 
+                    it('should not send recovered ERC20 to null address', async () => {
+                        await expect(
+                            zamaERC20
+                                .connect(admin)
+                                .recoverERC20(zamaERC20.address, SEND_AMOUNT, ethers.constants.AddressZero)
+                        ).to.be.revertedWithCustomError(zamaERC20, 'InvalidNullRecipient')
+                    })
+
                     it('should let DEFAULT_ADMIN_ROLE recover 0 $ZAMA from contract while unpaused', async () => {
                         const mockERC20ContractBalanceBefore = await zamaERC20.balanceOf(zamaERC20.address)
                         const mockERC20AliceBalanceBefore = await zamaERC20.balanceOf(alice.address)
@@ -909,6 +923,14 @@ describe('ZamaERC20 - Unit Test', () => {
                         expect(await ERC721Mock.ownerOf(TOKEN_ID)).to.eq(alice.address)
                     })
 
+                    it('should not send recovered ERC721 to null address', async () => {
+                        await expect(
+                            zamaERC20
+                                .connect(admin)
+                                .recoverERC721(ERC721Mock.address, TOKEN_ID, ethers.constants.AddressZero)
+                        ).to.be.revertedWithCustomError(zamaERC20, 'InvalidNullRecipient')
+                    })
+
                     it('should not let MINTER_ROLE recover ERC721Mock from contract while paused', async () => {
                         await expect(
                             zamaERC20.connect(alice).recoverERC721(ERC721Mock.address, TOKEN_ID, alice.address)