Merge pull request #1 from mento-protocol/fix/findings

Author: philbow61

.env.example

@@ -5,8 +5,9 @@ CELO_RPC_URL=https://forno.celo.org
 # Deployer address (Celo: Deployer 1)
 DEPLOYER=0xE23a4c6615669526Ab58E9c37088bee4eD2b2dEE
 
-# Owner of deployed recovery contracts (must differ from DEPLOYER to avoid burning nonces)
-RECOVERER=0x0000000000000000000000000000000000000000
+# Owner of deployed recovery contracts. Replace this placeholder with your
+# actual recoverer address; it must differ from DEPLOYER to avoid burning nonces.
+RECOVERER=0x0000000000000000000000000000000000000169
 
 # Deploy recovery contracts up to (and including) this nonce
 MAX_NONCE=68

src/RecoveryContract.sol

@@ -15,18 +15,24 @@ contract RecoveryContract is Ownable {
 
     error TransferFailed();
     error ExecutionFailed(bytes returnData);
+    error ZeroAddressRecipient();
+    error ZeroAddressTarget();
+    error NoCodeAtTarget(address target);
+    error EmptyExecution();
 
     /// @param _recoverer The address that owns this contract and can recover funds.
     constructor(address _recoverer) Ownable(_recoverer) {}
 
     /// @notice Recover native ETH held by this contract.
     function recoverETH(address payable to, uint256 amount) external onlyOwner {
+        if (to == address(0)) revert ZeroAddressRecipient();
         (bool success,) = to.call{value: amount}("");
         if (!success) revert TransferFailed();
     }
 
     /// @notice Recover any ERC20 token held by this contract.
     function recoverERC20(address token, address to, uint256 amount) external onlyOwner {
+        if (to == address(0)) revert ZeroAddressRecipient();
         IERC20(token).safeTransfer(to, amount);
     }
 
@@ -35,6 +41,12 @@ contract RecoveryContract is Ownable {
     /// @param value ETH value to send with the call.
     /// @param data Calldata to execute (e.g. ERC20.transfer, ERC721.transferFrom).
     function execute(address target, uint256 value, bytes calldata data) external onlyOwner returns (bytes memory) {
+        if (target == address(0)) revert ZeroAddressTarget();
+        if (target.code.length == 0) {
+            if (data.length > 0) revert NoCodeAtTarget(target);
+            if (value == 0) revert EmptyExecution();
+        }
+
         (bool success, bytes memory result) = target.call{value: value}(data);
         if (!success) revert ExecutionFailed(result);
         return result;

test/RecoveryContract.t.sol

@@ -48,6 +48,14 @@ contract RecoveryContractTest is Test {
         assertEq(address(rc).balance, 1.5 ether);
     }
 
+    function test_recoverETH_zeroRecipientReverts() public {
+        vm.deal(address(rc), 1 ether);
+
+        vm.prank(OWNER);
+        vm.expectRevert(abi.encodeWithSelector(RecoveryContract.ZeroAddressRecipient.selector));
+        rc.recoverETH(payable(address(0)), 1 ether);
+    }
+
     function test_recoverETH_unauthorized() public {
         vm.deal(address(rc), 1 ether);
 
@@ -68,6 +76,14 @@ contract RecoveryContractTest is Test {
         assertEq(token.balanceOf(address(rc)), 0);
     }
 
+    function test_recoverERC20_zeroRecipientReverts() public {
+        token.mint(address(rc), 1000e18);
+
+        vm.prank(OWNER);
+        vm.expectRevert(abi.encodeWithSelector(RecoveryContract.ZeroAddressRecipient.selector));
+        rc.recoverERC20(address(token), address(0), 1000e18);
+    }
+
     function test_recoverERC20_unauthorized() public {
         token.mint(address(rc), 1000e18);
 
@@ -96,6 +112,28 @@ contract RecoveryContractTest is Test {
         assertEq(nft.ownerOf(42), RECIPIENT);
     }
 
+    function test_execute_zeroTargetReverts() public {
+        vm.prank(OWNER);
+        vm.expectRevert(abi.encodeWithSelector(RecoveryContract.ZeroAddressTarget.selector));
+        rc.execute(address(0), 0, "");
+    }
+
+    function test_execute_noCodeWithCalldataReverts() public {
+        address noCodeTarget = makeAddr("noCodeTarget");
+
+        vm.prank(OWNER);
+        vm.expectRevert(abi.encodeWithSelector(RecoveryContract.NoCodeAtTarget.selector, noCodeTarget));
+        rc.execute(noCodeTarget, 0, hex"1234");
+    }
+
+    function test_execute_noCodeNoValueReverts() public {
+        address noCodeTarget = makeAddr("noCodeTarget");
+
+        vm.prank(OWNER);
+        vm.expectRevert(abi.encodeWithSelector(RecoveryContract.EmptyExecution.selector));
+        rc.execute(noCodeTarget, 0, "");
+    }
+
     function test_execute_unauthorized() public {
         vm.deal(address(rc), 1 ether);