fix: fix minor issues found in audit (ExocoreNetwork#82)

* fix: use user instead of msg.sender in internal function

* fix: use calldata for unmodified parameter

* fix: check msg.value is zero when payByApp is true

* fix: use correct refund address

* fix: initialize reentrancy guard for Exocapsule

* fix: add withdrawNonBeaconChainETHFromCapsule to support withdrawing non beacon chain eth

* fix: check msg.value should be 0 for Bootstrap payable functions

* fix: init OAppCoreUpgradeable for Bootstrap

* fix: mark capsule owner as payable

src/core/Bootstrap.sol

@@ -89,6 +89,7 @@ contract Bootstrap is
         // set can not sign without the chain, the owner is likely to be an EOA or a
         // contract controlled by one.
         _transferOwnership(owner);
+        __OAppCore_init_unchained(owner);
         __Pausable_init_unchained();
         __ReentrancyGuard_init_unchained();
     }
@@ -325,6 +326,9 @@ contract Bootstrap is
         isValidAmount(amount)
         nonReentrant // interacts with Vault
     {
+        if (msg.value > 0) {
+            revert Errors.NonZeroValue();
+        }
         _deposit(msg.sender, token, amount);
     }
 
@@ -365,6 +369,9 @@ contract Bootstrap is
         isValidAmount(amount)
         nonReentrant // interacts with Vault
     {
+        if (msg.value > 0) {
+            revert Errors.NonZeroValue();
+        }
         _withdraw(msg.sender, token, amount);
     }
 
@@ -428,6 +435,9 @@ contract Bootstrap is
         isValidBech32Address(validator)
     // does not need a reentrancy guard
     {
+        if (msg.value > 0) {
+            revert Errors.NonZeroValue();
+        }
         _delegateTo(msg.sender, validator, token, amount);
     }
 
@@ -447,7 +457,7 @@ contract Bootstrap is
         // validator can't be frozen and amount can't be negative
         // asset validity has been checked.
         // now check amounts.
-        uint256 withdrawable = withdrawableAmounts[msg.sender][token];
+        uint256 withdrawable = withdrawableAmounts[user][token];
         if (withdrawable < amount) {
             revert Errors.BootstrapInsufficientWithdrawableBalance();
         }
@@ -470,6 +480,9 @@ contract Bootstrap is
         isValidBech32Address(validator)
     // does not need a reentrancy guard
     {
+        if (msg.value > 0) {
+            revert Errors.NonZeroValue();
+        }
         _undelegateFrom(msg.sender, validator, token, amount);
     }
 
@@ -518,6 +531,9 @@ contract Bootstrap is
         isValidBech32Address(validator)
         nonReentrant // because it interacts with vault in deposit
     {
+        if (msg.value > 0) {
+            revert Errors.NonZeroValue();
+        }
         _deposit(msg.sender, token, amount);
         _delegateTo(msg.sender, validator, token, amount);
     }

src/core/ClientChainGateway.sol

@@ -115,7 +115,7 @@ contract ClientChainGateway is
     }
 
     /// @inheritdoc IClientChainGateway
-    function quote(bytes memory _message) public view returns (uint256 nativeFee) {
+    function quote(bytes calldata _message) public view returns (uint256 nativeFee) {
         bytes memory options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(
             DESTINATION_GAS_LIMIT, DESTINATION_MSG_VALUE
         ).addExecutorOrderedExecutionOption();

src/core/CustomProxyAdmin.sol

@@ -32,7 +32,7 @@ contract CustomProxyAdmin is Initializable, ProxyAdmin {
     /// @param implementation The address of the new implementation contract.
     /// @param data The data to be passed to the new implementation contract.
     /// @dev This function can only be called by the proxy to upgrade itself, exactly once.
-    function changeImplementation(address proxy, address implementation, bytes memory data) public virtual {
+    function changeImplementation(address proxy, address implementation, bytes calldata data) public virtual {
         if (msg.sender != bootstrapper) {
             revert Errors.CustomProxyAdminOnlyCalledFromBootstrapper();
         }

src/core/ExoCapsule.sol

@@ -148,7 +148,7 @@ contract ExoCapsule is ReentrancyGuardUpgradeable, ExoCapsuleStorage, IExoCapsul
     }
 
     /// @inheritdoc IExoCapsule
-    function initialize(address gateway_, address capsuleOwner_, address beaconOracle_) external initializer {
+    function initialize(address gateway_, address payable capsuleOwner_, address beaconOracle_) external initializer {
         require(gateway_ != address(0), "ExoCapsule: gateway address can not be empty");
         require(capsuleOwner_ != address(0), "ExoCapsule: capsule owner address can not be empty");
         require(beaconOracle_ != address(0), "ExoCapsule: beacon chain oracle address should not be empty");
@@ -157,6 +157,8 @@ contract ExoCapsule is ReentrancyGuardUpgradeable, ExoCapsuleStorage, IExoCapsul
         beaconOracle = IBeaconChainOracle(beaconOracle_);
         capsuleOwner = capsuleOwner_;
 
+        __ReentrancyGuard_init_unchained();
+
         emit RestakingActivated(capsuleOwner);
     }
 
@@ -266,11 +268,14 @@ contract ExoCapsule is ReentrancyGuardUpgradeable, ExoCapsuleStorage, IExoCapsul
     }
 
     /// @notice Withdraws the nonBeaconChainETHBalance
-    /// @dev This function must be called through the gateway. @param amount must be greater than
+    /// @dev This function must be called through the gateway. @param amountToWithdraw can not be greater than
     /// the available nonBeaconChainETHBalance.
-    /// @param recipient The destination address to which the ETH are sent.
+    /// @param recipient The payable destination address to which the ETH are sent.
     /// @param amountToWithdraw The amount to withdraw.
-    function withdrawNonBeaconChainETHBalance(address recipient, uint256 amountToWithdraw) external onlyGateway {
+    function withdrawNonBeaconChainETHBalance(address payable recipient, uint256 amountToWithdraw)
+        external
+        onlyGateway
+    {
         require(
             amountToWithdraw <= nonBeaconChainETHBalance,
             "ExoCapsule.withdrawNonBeaconChainETHBalance: amountToWithdraw is greater than nonBeaconChainETHBalance"
@@ -346,10 +351,10 @@ contract ExoCapsule is ReentrancyGuardUpgradeable, ExoCapsuleStorage, IExoCapsul
     }
 
     /// @dev Sends @param amountWei of ETH to the @param recipient.
-    /// @param recipient The address of the recipient.
+    /// @param recipient The address of the payable recipient.
     /// @param amountWei The amount of ETH to send, in wei.
     // slither-disable-next-line arbitrary-send-eth
-    function _sendETH(address recipient, uint256 amountWei) internal nonReentrant {
+    function _sendETH(address payable recipient, uint256 amountWei) internal nonReentrant {
         (bool sent,) = recipient.call{value: amountWei}("");
         if (!sent) {
             revert WithdrawalFailure(capsuleOwner, recipient, amountWei);

src/core/ExocoreGateway.sol

@@ -600,13 +600,14 @@ contract ExocoreGateway is
         ).addExecutorOrderedExecutionOption();
         MessagingFee memory fee = _quote(srcChainId, payload, options, false);
 
+        address refundAddress = payByApp ? address(this) : msg.sender;
         MessagingReceipt memory receipt =
-            _lzSend(srcChainId, payload, options, MessagingFee(fee.nativeFee, 0), msg.sender, payByApp);
+            _lzSend(srcChainId, payload, options, MessagingFee(fee.nativeFee, 0), refundAddress, payByApp);
         emit MessageSent(act, receipt.guid, receipt.nonce, receipt.fee.nativeFee);
     }
 
     /// @inheritdoc IExocoreGateway
-    function quote(uint32 srcChainid, bytes memory _message) public view returns (uint256 nativeFee) {
+    function quote(uint32 srcChainid, bytes calldata _message) public view returns (uint256 nativeFee) {
         bytes memory options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(
             DESTINATION_GAS_LIMIT, DESTINATION_MSG_VALUE
         ).addExecutorOrderedExecutionOption();

src/core/NativeRestakingController.sol

@@ -63,6 +63,7 @@ abstract contract NativeRestakingController is
     }
 
     /// @notice Creates a new ExoCapsule contract for the message sender.
+    /// @notice The message sender must be payable
     /// @return The address of the newly created ExoCapsule contract.
     // The bytecode returned by `BEACON_PROXY_BYTECODE` and `EXO_CAPSULE_BEACON` address are actually fixed size of byte
     // array, so it would not cause collision for encodePacked
@@ -82,7 +83,7 @@ abstract contract NativeRestakingController is
 
         // we follow check-effects-interactions pattern to write state before external call
         ownerToCapsule[msg.sender] = capsule;
-        capsule.initialize(address(this), msg.sender, BEACON_ORACLE_ADDRESS);
+        capsule.initialize(address(this), payable(msg.sender), BEACON_ORACLE_ADDRESS);
 
         emit CapsuleCreated(msg.sender, address(capsule));
 
@@ -130,4 +131,18 @@ abstract contract NativeRestakingController is
         }
     }
 
+    /// @notice Withdraws the nonBeaconChainETHBalance from the ExoCapsule contract.
+    /// @dev @param amountToWithdraw can not be greater than the available nonBeaconChainETHBalance.
+    /// @param recipient The payable destination address to which the ETH are sent.
+    /// @param amountToWithdraw The amount to withdraw.
+    function withdrawNonBeaconChainETHFromCapsule(address payable recipient, uint256 amountToWithdraw)
+        external
+        whenNotPaused
+        nonReentrant
+        nativeRestakingEnabled
+    {
+        IExoCapsule capsule = _getCapsule(msg.sender);
+        capsule.withdrawNonBeaconChainETHBalance(recipient, amountToWithdraw);
+    }
+
 }

src/interfaces/IExoCapsule.sol

@@ -11,9 +11,9 @@ interface IExoCapsule {
 
     /// @notice Initializes the ExoCapsule contract with the given parameters.
     /// @param gateway The address of the ClientChainGateway contract.
-    /// @param capsuleOwner The address of the ExoCapsule owner.
+    /// @param capsuleOwner The payable address of the ExoCapsule owner.
     /// @param beaconOracle The address of the BeaconOracle contract.
-    function initialize(address gateway, address capsuleOwner, address beaconOracle) external;
+    function initialize(address gateway, address payable capsuleOwner, address beaconOracle) external;
 
     /// @notice Verifies the deposit proof and returns the amount of deposit.
     /// @param validatorContainer The validator container.
@@ -45,12 +45,17 @@ interface IExoCapsule {
         BeaconChainProofs.WithdrawalProof calldata withdrawalProof
     ) external returns (bool partialWithdrawal, uint256 withdrawalAmount);
 
-    /// @notice Allows the owner to withdraw the specified amount to the recipient.
+    /// @notice Allows the owner to withdraw the specified unlocked staked ETH to the recipient.
     /// @dev The amount must be available in the withdrawable balance.
     /// @param amount The amount to withdraw.
     /// @param recipient The recipient address.
     function withdraw(uint256 amount, address payable recipient) external;
 
+    /// @notice Withdraws the nonBeaconChainETHBalance
+    /// @param recipient The payable destination address to which the ETH are sent.
+    /// @param amountToWithdraw The amount to withdraw.
+    function withdrawNonBeaconChainETHBalance(address payable recipient, uint256 amountToWithdraw) external;
+
     /// @notice Updates the principal balance of the ExoCapsule.
     /// @param lastlyUpdatedPrincipalBalance The final principal balance.
     function updatePrincipalBalance(uint256 lastlyUpdatedPrincipalBalance) external;

src/libraries/Errors.sol

@@ -23,6 +23,9 @@ library Errors {
     /// @dev Thrown when passed-in amount is zero
     error ZeroAmount();
 
+    /// @dev Thrown when the passed-in value is not zero
+    error NonZeroValue();
+
     /// @dev Thrown wehn the passed-in value is zero
     /// @dev This is used when the value in question is not an amount
     error ZeroValue();

src/lzApp/OAppSenderUpgradeable.sol

@@ -19,7 +19,7 @@ abstract contract OAppSenderUpgradeable is OAppCoreUpgradeable {
     using SafeERC20 for IERC20;
 
     // Custom error messages
-    error NotExactNativeFee(uint256 msgValue);
+    error IncorrectNativeFee(uint256 msgValue);
     error LzTokenUnavailable();
 
     // @dev The version of the OAppSender implementation.
@@ -70,7 +70,7 @@ abstract contract OAppSenderUpgradeable is OAppCoreUpgradeable {
      *      - nativeFee: The native fee.
      *      - lzTokenFee: The lzToken fee.
      * @param _refundAddress The address to receive any excess fee values sent to the endpoint.
-     * @param byApp Whether the native fee is paid by the app itself or by the app caller,
+     * @param payByApp Whether the native fee is paid by the app itself or by the app caller,
      * if byApp is true, app caller does not need to specify msg.value to pay for the native fee.
      * @return receipt The receipt for the sent message.
      *      - guid: The unique identifier for the sent message.
@@ -83,11 +83,11 @@ abstract contract OAppSenderUpgradeable is OAppCoreUpgradeable {
         bytes memory _options,
         MessagingFee memory _fee,
         address _refundAddress,
-        bool byApp
+        bool payByApp
     ) internal virtual returns (MessagingReceipt memory receipt) {
         // @dev Push corresponding fees to the endpoint, any excess is sent back to the _refundAddress from the
         // endpoint.
-        uint256 messageValue = _payNative(_fee.nativeFee, byApp);
+        uint256 messageValue = _payNative(_fee.nativeFee, payByApp);
         if (_fee.lzTokenFee > 0) {
             _payLzToken(_fee.lzTokenFee);
         }
@@ -102,7 +102,7 @@ abstract contract OAppSenderUpgradeable is OAppCoreUpgradeable {
     /**
      * @dev Internal function to pay the native fee associated with the message.
      * @param _nativeFee The native fee to be paid.
-     * @param byApp Whether the native fee is paid by the app itself or by the app caller,
+     * @param payByApp Whether the native fee is paid by the app itself or by the app caller,
      * if byApp is true, do not check that the msg.value is equal to nativeFee.
      * @return nativeFee The amount of native currency paid.
      *
@@ -112,9 +112,9 @@ abstract contract OAppSenderUpgradeable is OAppCoreUpgradeable {
      * @dev Some EVMs use an ERC20 as a method for paying transactions/gasFees.
      * @dev The endpoint is EITHER/OR, ie. it will NOT support both types of native payment at a time.
      */
-    function _payNative(uint256 _nativeFee, bool byApp) internal virtual returns (uint256 nativeFee) {
-        if (!byApp && msg.value != _nativeFee) {
-            revert NotExactNativeFee(msg.value);
+    function _payNative(uint256 _nativeFee, bool payByApp) internal virtual returns (uint256 nativeFee) {
+        if ((!payByApp && msg.value != _nativeFee) || (payByApp && msg.value != 0)) {
+            revert IncorrectNativeFee(msg.value);
         }
         return _nativeFee;
     }

src/storage/ExoCapsuleStorage.sol

@@ -59,7 +59,7 @@ contract ExoCapsuleStorage {
     uint256 public nonBeaconChainETHBalance;
 
     /// @notice The owner of the ExoCapsule.
-    address public capsuleOwner;
+    address payable public capsuleOwner;
 
     /// @notice The address of the NativeRestakingController contract.
     INativeRestakingController public gateway;

test/foundry/DepositWithdrawPrinciple.t.sol

@@ -461,7 +461,7 @@ contract DepositWithdrawPrincipalTest is ExocoreDeployer {
         assertEq(address(clientGateway.ownerToCapsule(depositor.addr)), address(capsule));
 
         /// initialize replaced capsule
-        capsule.initialize(address(clientGateway), depositor.addr, address(beaconOracle));
+        capsule.initialize(address(clientGateway), payable(depositor.addr), address(beaconOracle));
     }
 
     function _testNativeWithdraw(Player memory withdrawer, Player memory relayer, uint256 lastlyUpdatedPrincipalBalance)

test/foundry/unit/Bootstrap.t.sol

@@ -16,6 +16,7 @@ import {BootstrapStorage} from "src/storage/BootstrapStorage.sol";
 import {GatewayStorage} from "src/storage/GatewayStorage.sol";
 
 import "@layerzerolabs/lz-evm-protocol-v2/contracts/libs/GUID.sol";
+import "src/libraries/Errors.sol";
 
 import {IBeacon} from "@openzeppelin/contracts/proxy/beacon/IBeacon.sol";
 import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
@@ -1191,4 +1192,50 @@ contract BootstrapTest is Test {
         bootstrap.claim(address(myToken), amounts[0] + 5, addrs[0]);
     }
 
+    function test23_RevertWhen_Deposit_WithEther() public {
+        vm.startPrank(addrs[0]);
+        vm.deal(addrs[0], 1 ether);
+        vm.expectRevert(Errors.NonZeroValue.selector);
+        bootstrap.deposit{value: 0.1 ether}(address(myToken), amounts[0]);
+        vm.stopPrank();
+    }
+
+    function test23_RevertWhen_WithdrawPrincipalFromExocore_WithEther() public {
+        vm.startPrank(addrs[0]);
+        vm.deal(addrs[0], 1 ether);
+        vm.expectRevert(Errors.NonZeroValue.selector);
+        bootstrap.withdrawPrincipalFromExocore{value: 0.1 ether}(address(myToken), amounts[0]);
+        vm.stopPrank();
+    }
+
+    function test23_RevertWhen_DelegateTo_WithEther() public {
+        vm.startPrank(addrs[0]);
+        vm.deal(addrs[0], 1 ether);
+        vm.expectRevert(Errors.NonZeroValue.selector);
+        bootstrap.delegateTo{value: 0.1 ether}(
+            "exo13hasr43vvq8v44xpzh0l6yuym4kca98f87j7ac", address(myToken), amounts[0]
+        );
+        vm.stopPrank();
+    }
+
+    function test23_RevertWhen_UndelegateFrom_WithEther() public {
+        vm.startPrank(addrs[0]);
+        vm.deal(addrs[0], 1 ether);
+        vm.expectRevert(Errors.NonZeroValue.selector);
+        bootstrap.undelegateFrom{value: 0.1 ether}(
+            "exo13hasr43vvq8v44xpzh0l6yuym4kca98f87j7ac", address(myToken), amounts[0]
+        );
+        vm.stopPrank();
+    }
+
+    function test23_RevertWhen_DepositThenDelegateTo_WithEther() public {
+        vm.startPrank(addrs[0]);
+        vm.deal(addrs[0], 1 ether);
+        vm.expectRevert(Errors.NonZeroValue.selector);
+        bootstrap.depositThenDelegateTo{value: 0.1 ether}(
+            address(myToken), amounts[0], "exo13hasr43vvq8v44xpzh0l6yuym4kca98f87j7ac"
+        );
+        vm.stopPrank();
+    }
+
 }