feat(Governance): dedicated withdraw method and safety checks (#220)

* fix(Governance): dedicated withdraw method and safety checks * Update contracts/governance/InterchainGovernance.sol Co-authored-by: Milap Sheth <milap@axelar.network> * Update contracts/interfaces/IInterchainGovernance.sol Co-authored-by: Milap Sheth <milap@axelar.network> * added tests * Apply suggestions from code review * addressed comments * addressed comments --------- Co-authored-by: Milap Sheth <milap@axelar.network> Co-authored-by: Dean Amiel <dean@axelar.network>
axelarnetwork · Aug 3, 2023 · 519a1b9 · 519a1b9
1 parent 9a64b69
commit 519a1b9
Show file tree

Hide file tree

Showing 9 changed files with 254 additions and 238 deletions.
diff --git a/contracts/governance/AxelarServiceGovernance.sol b/contracts/governance/AxelarServiceGovernance.sol
@@ -50,7 +50,7 @@ contract AxelarServiceGovernance is InterchainGovernance, MultisigBase, IAxelarS
         bytes calldata callData,
         uint256 nativeValue
     ) external payable onlySigners {
-        bytes32 proposalHash = keccak256(abi.encodePacked(target, callData, nativeValue));
+        bytes32 proposalHash = _getProposalHash(target, callData, nativeValue);
 
         if (!multisigApprovals[proposalHash]) revert NotApproved();
 
@@ -63,46 +63,43 @@ contract AxelarServiceGovernance is InterchainGovernance, MultisigBase, IAxelarS
 
     /**
      * @notice Internal function to process a governance command
-     * @param commandId The id of the command
+     * @param commandType The type of the command
      * @param target The target address the proposal will call
      * @param callData The data the encodes the function and arguments to call on the target contract
      * @param nativeValue The value of native token to be sent to the target contract
      * @param eta The time after which the proposal can be executed
      */
     function _processCommand(
-        uint256 commandId,
+        uint256 commandType,
         address target,
         bytes memory callData,
         uint256 nativeValue,
         uint256 eta
     ) internal override {
-        if (commandId > uint256(type(ServiceGovernanceCommand).max)) {
-            revert InvalidCommand();
-        }
+        bytes32 proposalHash = _getProposalHash(target, callData, nativeValue);
 
-        ServiceGovernanceCommand command = ServiceGovernanceCommand(commandId);
-        bytes32 proposalHash = keccak256(abi.encodePacked(target, callData, nativeValue));
-
-        if (command == ServiceGovernanceCommand.ScheduleTimeLockProposal) {
+        if (commandType == uint256(ServiceGovernanceCommand.ScheduleTimeLockProposal)) {
             eta = _scheduleTimeLock(proposalHash, eta);
 
             emit ProposalScheduled(proposalHash, target, callData, nativeValue, eta);
             return;
-        } else if (command == ServiceGovernanceCommand.CancelTimeLockProposal) {
+        } else if (commandType == uint256(ServiceGovernanceCommand.CancelTimeLockProposal)) {
             _cancelTimeLock(proposalHash);
 
             emit ProposalCancelled(proposalHash, target, callData, nativeValue, eta);
             return;
-        } else if (command == ServiceGovernanceCommand.ApproveMultisigProposal) {
+        } else if (commandType == uint256(ServiceGovernanceCommand.ApproveMultisigProposal)) {
             multisigApprovals[proposalHash] = true;
 
             emit MultisigApproved(proposalHash, target, callData, nativeValue);
             return;
-        } else if (command == ServiceGovernanceCommand.CancelMultisigApproval) {
+        } else if (commandType == uint256(ServiceGovernanceCommand.CancelMultisigApproval)) {
             multisigApprovals[proposalHash] = false;
 
             emit MultisigCancelled(proposalHash, target, callData, nativeValue);
             return;
+        } else {
+            revert InvalidCommand();
         }
     }
 }
diff --git a/contracts/governance/InterchainGovernance.sol b/contracts/governance/InterchainGovernance.sol
@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 
 import { AxelarExecutable } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/executable/AxelarExecutable.sol';
 import { TimeLock } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/utils/TimeLock.sol';
+import { SafeNativeTransfer } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/utils/SafeTransfer.sol';
 import { IInterchainGovernance } from '../interfaces/IInterchainGovernance.sol';
 import { Caller } from '../util/Caller.sol';
 
@@ -13,6 +14,8 @@ import { Caller } from '../util/Caller.sol';
  * to create, cancel, and execute governance proposals.
  */
 contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchainGovernance {
+    using SafeNativeTransfer for address;
+
     enum GovernanceCommand {
         ScheduleTimeLockProposal,
         CancelTimeLockProposal
@@ -42,6 +45,27 @@ contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchain
         governanceAddressHash = keccak256(bytes(governanceAddress_));
     }
 
+    /**
+     * @notice Modifier to check if the caller is the governance contract
+     * @param sourceChain The source chain of the proposal, must equal the governance chain
+     * @param sourceAddress The source address of the proposal, must equal the governance address
+     */
+    modifier onlyGovernance(string calldata sourceChain, string calldata sourceAddress) {
+        if (keccak256(bytes(sourceChain)) != governanceChainHash || keccak256(bytes(sourceAddress)) != governanceAddressHash)
+            revert NotGovernance();
+
+        _;
+    }
+
+    /**
+     * @notice Modifier to check if the caller is the contract itself
+     */
+    modifier onlySelf() {
+        if (msg.sender != address(this)) revert NotSelf();
+
+        _;
+    }
+
     /**
      * @notice Returns the ETA of a proposal
      * @param target The address of the contract targeted by the proposal
@@ -70,14 +94,24 @@ contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchain
         bytes calldata callData,
         uint256 nativeValue
     ) external payable {
-        bytes32 proposalHash = keccak256(abi.encodePacked(target, callData, nativeValue));
+        bytes32 proposalHash = _getProposalHash(target, callData, nativeValue);
 
         _finalizeTimeLock(proposalHash);
         _call(target, callData, nativeValue);
 
         emit ProposalExecuted(proposalHash, target, callData, nativeValue, block.timestamp);
     }
 
+    /**
+     * @notice Withdraws native token from the contract
+     * @param recipient The address to send the native token to
+     * @param amount The amount of native token to send
+     * @dev This function is only callable by the contract itself after passing according proposal
+     */
+    function withdraw(address recipient, uint256 amount) external onlySelf {
+        recipient.safeNativeTransfer(amount);
+    }
+
     /**
      * @notice Internal function to execute a proposal action
      * @param sourceChain The source chain of the proposal, must equal the governance chain
@@ -88,10 +122,7 @@ contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchain
         string calldata sourceChain,
         string calldata sourceAddress,
         bytes calldata payload
-    ) internal override {
-        if (keccak256(bytes(sourceChain)) != governanceChainHash || keccak256(bytes(sourceAddress)) != governanceAddressHash)
-            revert NotGovernance();
-
+    ) internal override onlyGovernance(sourceChain, sourceAddress) {
         (uint256 command, address target, bytes memory callData, uint256 nativeValue, uint256 eta) = abi.decode(
             payload,
             (uint256, address, bytes, uint256, uint256)
@@ -104,36 +135,33 @@ contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchain
 
     /**
      * @notice Internal function to process a governance command
-     * @param commandId The id of the command, 0 for proposal creation and 1 for proposal cancellation
+     * @param commandType The type of the command, 0 for proposal creation and 1 for proposal cancellation
      * @param target The target address the proposal will call
      * @param callData The data the encodes the function and arguments to call on the target contract
      * @param nativeValue The nativeValue of native token to be sent to the target contract
      * @param eta The time after which the proposal can be executed
      */
     function _processCommand(
-        uint256 commandId,
+        uint256 commandType,
         address target,
         bytes memory callData,
         uint256 nativeValue,
         uint256 eta
     ) internal virtual {
-        if (commandId > uint256(type(GovernanceCommand).max)) {
-            revert InvalidCommand();
-        }
-
-        GovernanceCommand command = GovernanceCommand(commandId);
         bytes32 proposalHash = _getProposalHash(target, callData, nativeValue);
 
-        if (command == GovernanceCommand.ScheduleTimeLockProposal) {
+        if (commandType == uint256(GovernanceCommand.ScheduleTimeLockProposal)) {
             eta = _scheduleTimeLock(proposalHash, eta);
 
             emit ProposalScheduled(proposalHash, target, callData, nativeValue, eta);
             return;
-        } else if (command == GovernanceCommand.CancelTimeLockProposal) {
+        } else if (commandType == uint256(GovernanceCommand.CancelTimeLockProposal)) {
             _cancelTimeLock(proposalHash);
 
             emit ProposalCancelled(proposalHash, target, callData, nativeValue, eta);
             return;
+        } else {
+            revert InvalidCommand();
         }
     }
 
@@ -145,7 +173,7 @@ contract InterchainGovernance is AxelarExecutable, TimeLock, Caller, IInterchain
         bytes memory callData,
         uint256 nativeValue
     ) internal pure returns (bytes32) {
-        return keccak256(abi.encodePacked(target, callData, nativeValue));
+        return keccak256(abi.encode(target, callData, nativeValue));
     }
 
     /**

diff --git a/contracts/interfaces/IAxelarServiceGovernance.sol b/contracts/interfaces/IAxelarServiceGovernance.sol
@@ -16,6 +16,13 @@ interface IAxelarServiceGovernance is IMultisigBase, IInterchainGovernance {
     event MultisigCancelled(bytes32 indexed proposalHash, address indexed targetContract, bytes callData, uint256 nativeValue);
     event MultisigExecuted(bytes32 indexed proposalHash, address indexed targetContract, bytes callData, uint256 nativeValue);
 
+    /**
+     * @notice Returns whether a multisig proposal has been approved
+     * @param proposalHash The hash of the proposal
+     * @return bool Whether the proposal has been approved
+     */
+    function multisigApprovals(bytes32 proposalHash) external view returns (bool);
+
     /**
      * @notice Executes a multisig proposal
      * @param targetContract The target address the proposal will call

diff --git a/contracts/interfaces/ICaller.sol b/contracts/interfaces/ICaller.sol
@@ -3,6 +3,7 @@
 pragma solidity ^0.8.0;
 
 interface ICaller {
+    error InvalidContract(address target);
     error InsufficientBalance();
     error ExecutionFailed();
 }
diff --git a/contracts/interfaces/IInterchainGovernance.sol b/contracts/interfaces/IInterchainGovernance.sol
@@ -11,6 +11,7 @@ import { ICaller } from './ICaller.sol';
  */
 interface IInterchainGovernance is IAxelarExecutable, ICaller {
     error NotGovernance();
+    error NotSelf();
     error InvalidCommand();
     error InvalidTarget();
     error TokenNotSupported();
@@ -67,4 +68,12 @@ interface IInterchainGovernance is IAxelarExecutable, ICaller {
         bytes calldata callData,
         uint256 value
     ) external payable;
+
+    /**
+     * @notice Withdraws native token from the contract
+     * @param recipient The address to send the native token to
+     * @param amount The amount of native token to send
+     * @dev This function is only callable by the contract itself after passing according proposal
+     */
+    function withdraw(address recipient, uint256 amount) external;
 }
diff --git a/contracts/util/Caller.sol b/contracts/util/Caller.sol
@@ -2,9 +2,12 @@
 
 pragma solidity ^0.8.0;
 
+import { ContractAddress } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/utils/ContractAddress.sol';
 import { ICaller } from '../interfaces/ICaller.sol';
 
 contract Caller is ICaller {
+    using ContractAddress for address;
+
     /**
      * @dev Calls a target address with specified calldata and optionally sends value.
      */
@@ -13,6 +16,8 @@ contract Caller is ICaller {
         bytes calldata callData,
         uint256 nativeValue
     ) internal {
+        if (!target.isContract()) revert InvalidContract(target);
+
         if (nativeValue > address(this).balance) revert InsufficientBalance();
 
         (bool success, ) = target.call{ value: nativeValue }(callData);