Merge pull request #50 from perpetual-protocol/feat/internal-audit-1.5.0

fix internal audit 1.5.0 + add role: position manager

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Add `OtcMaker` contract.
 - Update `LimitOrderBook` for supporting whitelisted contract can call fillLimitOrder.
+- Update `ILimitOrderBook` for supporting new OrderType OtcMakerOrder.
 
 ## [1.4.3] - 2023-04-11
 ### Added

contracts/interface/ILimitOrderBook.sol

@@ -7,7 +7,8 @@ interface ILimitOrderBook {
  enum OrderType {
  LimitOrder,
  StopLossLimitOrder,
- TakeProfitLimitOrder
+ TakeProfitLimitOrder,
+ OtcMakerOrder
  }
 
  // Do NOT change the order of enum values because it will break backwards compatibility

contracts/interface/IOtcMaker.sol

@@ -13,22 +13,57 @@ interface IOtcMaker is IOtcMakerStruct, IOtcMakerEvent {
  // EXTERNAL NON-VIEW
  //
 
+ /// @notice Opens a position for trader by the given limit order,
+ /// using the specified JIT liquidity parameters and signature.
+ /// @dev This function can only be called by the designated caller.
+ /// @param limitOrderParams The parameters of the limit order.
+ /// @param jitLiquidityParams The JIT liquidity parameters.
+ /// @param signature The signature associated with the limit order.
+ /// @return exchangedPositionSize The exchanged position size of OTCMaker.
+ /// @return exchangedPositionNotional The exchanged position notional of OTCMaker.
  function openPositionFor(
  ILimitOrderBook.LimitOrder calldata limitOrderParams,
  JitLiquidityParams calldata jitLiquidityParams,
  bytes calldata signature
- ) external;
-
+ ) external returns (int256 exchangedPositionSize, int256 exchangedPositionNotional);
+
+ /// @notice Opens a position in the clearing house using the provided parameters.
+ /// @dev This function can only be called by the designated position manager.
+ /// @dev Delegates the call to the clearing house to open the position.
+ /// @param params The parameters for opening the position.
+ /// @return base The amount of baseToken the taker got or spent
+ /// @return quote The amount of quoteToken the taker got or spent
  function openPosition(IClearingHouse.OpenPositionParams calldata params)
  external
  returns (uint256 base, uint256 quote);
 
+ /// @notice Deposits a specified amount of tokens into the vault.
+ /// @dev This function can only be called by the designated fund owner.
+ /// @param token The address of the token to deposit.
+ /// @param amount The amount of tokens to deposit.
  function deposit(address token, uint256 amount) external;
 
+ /// @notice Withdraws a specified amount of tokens from the vault and transfers them to the fund owner.
+ /// @dev This function can only be called by the designated fund owner.
+ /// @dev Delegates the call to the vault to withdraw the tokens and transfers them to the fund owner.
+ /// @param token The address of the token to withdraw.
+ /// @param amount The amount of tokens to withdraw.
  function withdraw(address token, uint256 amount) external;
 
+ /// @notice Withdraws the entire balance of a specific token from the contract and transfers it to the fund owner.
+ /// @dev This function can only be called by the designated fund owner.
+ /// @dev Retrieves the balance of the specified token and transfers the full amount to the fund owner.
+ /// @param token The address of the token to withdraw.
  function withdrawToken(address token) external;
 
+ /// @notice Claims a specific week of rewards for a liquidity provider using Merkle proofs.
+ /// @dev This function can only be called by the designated fund owner.
+ /// @dev Delegates the call to the MerkleRedeem contract to claim rewards for the liquidity provider.
+ /// @param merkleRedeem The address of the MerkleRedeem contract.
+ /// @param liquidityProvider The address of the liquidity provider.
+ /// @param week The week number to claim rewards for.
+ /// @param claimedBalance The claimed amount.
+ /// @param _merkleProof The Merkle proofs for the verification.
  function claimWeek(
  address merkleRedeem,
  address liquidityProvider,
@@ -37,16 +72,16 @@ interface IOtcMaker is IOtcMakerStruct, IOtcMakerEvent {
  bytes32[] calldata _merkleProof
  ) external;
 
- function setCaller(address newCaller) external;
-
- function setMarginRatioLimit(uint24 openMarginRatioLimitArg) external;
-
  //
  // EXTERNAL VIEW
  //
 
  function getCaller() external view returns (address);
 
+ function getFundOwner() external view returns (address);
+
+ function getPositionManager() external view returns (address);
+
  function getClearingHouse() external view returns (address);
 
  function getLimitOrderBook() external view returns (address);

contracts/interface/IOtcMakerEvent.sol

@@ -3,5 +3,7 @@ pragma solidity 0.7.6;
 pragma abicoder v2;
 
 interface IOtcMakerEvent {
- event UpdateCaller(address oldCaller, address newCaller);
+ event CallerUpdated(address oldCaller, address newCaller);
+ event FundOwnerUpdated(address oldFundOwner, address newFundOwner);
+ event PositionManagerUpdated(address oldPositionManager, address newPositionManager);
 }

contracts/interface/IOtcMakerStruct.sol

@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity 0.7.6;
 
 interface IOtcMakerStruct {

contracts/limitOrder/LimitOrderBook.sol

@@ -107,10 +107,10 @@ contract LimitOrderBook is
  emit LimitOrderRewardVaultChanged(limitOrderRewardVaultArg);
  }
 
- function setWhitelistContractCaller(address caller, bool enable) external onlyOwner {
+ function setWhitelistContractCaller(address caller, bool isEnabled) external onlyOwner {
  // LOB_NCA: Not contract address
  require(caller.isContract(), "LOB_NCA");
- _whitelistedContractCaller[caller] = enable;
+ _whitelistedContractCaller[caller] = isEnabled;
  }
 
  /// @inheritdoc ILimitOrderBook
@@ -195,6 +195,13 @@ contract LimitOrderBook is
  );
  }
 
+ //
+ // EXTERNAL VIEW
+ //
+ function getOrderStatus(bytes32 orderHash) external view override returns (ILimitOrderBook.OrderStatus) {
+ return _ordersStatus[orderHash];
+ }
+
  //
  // PUBLIC VIEW
  //
@@ -206,10 +213,6 @@ contract LimitOrderBook is
  return _whitelistedContractCaller[caller];
  }
 
- function getOrderStatus(bytes32 orderHash) external view override returns (ILimitOrderBook.OrderStatus) {
- return _ordersStatus[orderHash];
- }
-
  //
  // INTERNAL NON-VIEW
  //
@@ -288,7 +291,10 @@ contract LimitOrderBook is
  }
 
  function _verifyTriggerPrice(LimitOrder memory order, uint80 roundIdWhenTriggered) internal view {
- if (order.orderType == ILimitOrderBook.OrderType.LimitOrder) {
+ if (
+ order.orderType == ILimitOrderBook.OrderType.LimitOrder ||
+ order.orderType == ILimitOrderBook.OrderType.OtcMakerOrder
+ ) {
  return;
  }
 

contracts/otcMaker/OtcMaker.sol

@@ -3,7 +3,8 @@ pragma solidity 0.7.6;
 pragma abicoder v2;
 
 import { SafeERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/SafeERC20Upgradeable.sol";
-import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/SafeERC20Upgradeable.sol";
+import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
+import { SignedSafeMathUpgradeable } from "@openzeppelin/contracts-upgradeable/math/SignedSafeMathUpgradeable.sol";
 import { ECDSAUpgradeable } from "@openzeppelin/contracts-upgradeable/cryptography/ECDSAUpgradeable.sol";
 import { EIP712Upgradeable } from "@openzeppelin/contracts-upgradeable/drafts/EIP712Upgradeable.sol";
 import { FullMath } from "@uniswap/v3-core/contracts/libraries/FullMath.sol";
@@ -14,15 +15,17 @@ import { IClearingHouse } from "@perp/curie-contract/contracts/interface/ICleari
 import { IClearingHouseConfig } from "@perp/curie-contract/contracts/interface/IClearingHouseConfig.sol";
 import { IVault } from "@perp/curie-contract/contracts/interface/IVault.sol";
 import { IAccountBalance } from "@perp/curie-contract/contracts/interface/IAccountBalance.sol";
+import { AccountMarket } from "@perp/curie-contract/contracts/lib/AccountMarket.sol";
 
 import { SafeOwnable } from "../base/SafeOwnable.sol";
 
 import { IMerkleRedeem } from "../interface/IMerkleRedeem.sol";
 import { IOtcMaker } from "../interface/IOtcMaker.sol";
 import { ILimitOrderBook } from "../interface/ILimitOrderBook.sol";
-import { OtcMakerStorageV1 } from "../storage/OtcMakerStorage.sol";
+import { OtcMakerStorageV2 } from "../storage/OtcMakerStorage.sol";
 
-contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV1 {
+contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV2 {
+ using SignedSafeMathUpgradeable for int256;
  using PerpMath for int256;
  using PerpMath for uint256;
  using PerpSafeCast for int256;
@@ -38,33 +41,62 @@ contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV
  _;
  }
 
+ modifier onlyFundOwner() {
+ // OM_NFO: not fund owner
+ require(_msgSender() == _fundOwner, "OM_NFO");
+ _;
+ }
+
+ modifier onlyPositionManager() {
+ // OM_NPM: not position manager
+ require(_msgSender() == _positionManager, "OM_NPM");
+ _;
+ }
+
  //
  // EXTERNAL NON-VIEW
  //
 
  function initialize(address clearingHouseArg, address limitOrderBookArg) external initializer {
  __SafeOwnable_init();
  _caller = _msgSender();
+ _fundOwner = _msgSender();
+ _positionManager = _msgSender();
  _clearingHouse = clearingHouseArg;
  _limitOrderBook = limitOrderBookArg;
  _vault = IClearingHouse(_clearingHouse).getVault();
  _accountBalance = IClearingHouse(_clearingHouse).getAccountBalance();
  }
 
- function setCaller(address newCaller) external override onlyOwner {
- // OM_ZA: zero address
- require(newCaller != address(0), "OM_ZA");
+ function setCaller(address newCaller) external onlyOwner {
+ _requireNonZeroAddress(newCaller);
+ address oldCaller = _caller;
  _caller = newCaller;
- emit UpdateCaller(_caller, newCaller);
+ emit CallerUpdated(oldCaller, newCaller);
+ }
+
+ function setFundOwner(address newFundOwner) external onlyOwner {
+ _requireNonZeroAddress(newFundOwner);
+ address oldFundOwner = _fundOwner;
+ _fundOwner = newFundOwner;
+ emit FundOwnerUpdated(oldFundOwner, newFundOwner);
+ }
+
+ function setPositionManager(address newPositionManager) external onlyOwner {
+ _requireNonZeroAddress(newPositionManager);
+ address oldPositionManager = _positionManager;
+ _positionManager = newPositionManager;
+ emit PositionManagerUpdated(oldPositionManager, newPositionManager);
  }
 
- function setMarginRatioLimit(uint24 marginRatioLimitArg) external override onlyOwner {
- require(marginRatioLimitArg > 62500 && marginRatioLimitArg < 1000000, "OM_IMR");
+ function setMarginRatioLimit(uint24 marginRatioLimitArg) external onlyOwner {
+ uint24 mmRatio = IClearingHouseConfig(IClearingHouse(_clearingHouse).getClearingHouseConfig()).getMmRatio();
+ require(marginRatioLimitArg > mmRatio && marginRatioLimitArg < 1000000, "OM_IMR");
  _marginRatioLimit = marginRatioLimitArg;
  }
 
- function deposit(address token, uint256 amount) external override onlyOwner {
- SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(token), owner(), address(this), amount);
+ function deposit(address token, uint256 amount) external override onlyFundOwner {
+ SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(token), _fundOwner, address(this), amount);
  IERC20Upgradeable(token).approve(_vault, amount);
  IVault(_vault).deposit(token, amount);
  }
@@ -73,9 +105,15 @@ contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV
  ILimitOrderBook.LimitOrder calldata limitOrderParams,
  JitLiquidityParams calldata jitLiquidityParams,
  bytes calldata signature
- ) external override onlyCaller {
- // OM_NLO: not limit order
- require(limitOrderParams.orderType == ILimitOrderBook.OrderType.LimitOrder, "OM_NLO");
+ ) external override onlyCaller returns (int256, int256) {
+ // OM_NOMO: not otc maker order
+ require(limitOrderParams.orderType == ILimitOrderBook.OrderType.OtcMakerOrder, "OM_NOMO");
+
+ // IAccountBalance -> get before takerPositionSize, takerPositionNotional
+ AccountMarket.Info memory accountInfoBefore = IAccountBalance(_accountBalance).getAccountInfo(
+ address(this),
+ limitOrderParams.baseToken
+ );
 
  // TODO should we set minBase & minQuote's percentage as a constant in contract?
  IClearingHouse.AddLiquidityResponse memory addLiquidityResponse = IClearingHouse(_clearingHouse).addLiquidity(
@@ -110,35 +148,45 @@ contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV
 
  // OM_IM: insufficient margin
  require(isMarginSufficient(), "OM_IM");
+
+ // IAccountBalance -> get after takerPositionSize, takerPositionNotional
+ AccountMarket.Info memory accountInfoAfter = IAccountBalance(_accountBalance).getAccountInfo(
+ address(this),
+ limitOrderParams.baseToken
+ );
+ return (
+ accountInfoAfter.takerPositionSize.sub(accountInfoBefore.takerPositionSize),
+ accountInfoAfter.takerOpenNotional.sub(accountInfoBefore.takerOpenNotional)
+ );
  }
 
  function openPosition(IClearingHouse.OpenPositionParams calldata params)
  external
  override
- onlyOwner
+ onlyPositionManager
  returns (uint256 base, uint256 quote)
  {
  return IClearingHouse(_clearingHouse).openPosition(params);
  }
 
- function withdraw(address token, uint256 amount) external override onlyOwner {
+ function withdraw(address token, uint256 amount) external override onlyFundOwner {
  IVault(_vault).withdraw(token, amount);
- SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(token), owner(), amount);
+ SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(token), _fundOwner, amount);
  }
 
- function withdrawToken(address token) external override onlyOwner {
+ function withdrawToken(address token) external override onlyFundOwner {
  uint256 amount = IERC20Upgradeable(token).balanceOf(address(this));
- SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(token), owner(), amount);
+ SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(token), _fundOwner, amount);
  }
 
  function claimWeek(
  address merkleRedeem,
  address liquidityProvider,
  uint256 week,
  uint256 claimedBalance,
- bytes32[] calldata _merkleProof
- ) external override onlyOwner {
- IMerkleRedeem(merkleRedeem).claimWeek(liquidityProvider, week, claimedBalance, _merkleProof);
+ bytes32[] calldata merkleProof
+ ) external override onlyFundOwner {
+ IMerkleRedeem(merkleRedeem).claimWeek(liquidityProvider, week, claimedBalance, merkleProof);
  }
 
  //
@@ -149,6 +197,14 @@ contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV
  return _caller;
  }
 
+ function getFundOwner() external view override returns (address) {
+ return _fundOwner;
+ }
+
+ function getPositionManager() external view override returns (address) {
+ return _positionManager;
+ }
+
  function getClearingHouse() external view override returns (address) {
  return _clearingHouse;
  }
@@ -189,4 +245,9 @@ contract OtcMaker is SafeOwnable, EIP712Upgradeable, IOtcMaker, OtcMakerStorageV
  //
  // INTERNAL PURE
  //
+
+ function _requireNonZeroAddress(address addressArg) internal pure {
+ // OM_ZA: zero address
+ require(addressArg != address(0), "OM_ZA");
+ }
 }

contracts/storage/OtcMakerStorage.sol

@@ -12,3 +12,8 @@ abstract contract OtcMakerStorageV1 {
  address internal _accountBalance;
  uint24 internal _marginRatioLimit;
 }
+
+abstract contract OtcMakerStorageV2 is OtcMakerStorageV1 {
+ address internal _fundOwner;
+ address internal _positionManager;
+}