Audit/hexens 2nd round part2.2 (#209)

* optimization: [hexen-s8] added uncheck for binHelper#getSharesAndEffectiveAmountsIn

* refactor: make vault extends Ownable2Step instead of Ownable in case ownableship is transferred by mistake

* refactor: [hexsen-s3] update PoolManagerOwner to support 2 step poolManager ownership transfer

* optimization: added events when start, finish ownership transfer

.forge-snapshots/BinCustomCurveHookTest#test_Swap_CustomCurve.snap

@@ -1 +1 @@
-142368
+142478

.forge-snapshots/BinMintBurnFeeHookTest#test_Burn.snap

@@ -1 +1 @@
-170035
+170145

.forge-snapshots/BinMintBurnFeeHookTest#test_Mint.snap

@@ -1 +1 @@
-410226
+410336

.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap

@@ -1 +1 @@
-133857
+133892

.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap

@@ -1 +1 @@
-142673
+142717

.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap

@@ -1 +1 @@
-289648
+289683

.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap

@@ -1 +1 @@
-127029
+127065

.forge-snapshots/BinPoolManagerTest#testGasDonate.snap

@@ -1 +1 @@
-118603
+118691

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap

@@ -1 +1 @@
-968387
+968475

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap

@@ -1 +1 @@
-327699
+327787

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap

@@ -1 +1 @@
-337423
+337511

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap

@@ -1 +1 @@
-139974
+140062

.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap

@@ -1 +1 @@
-173032
+173098

.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap

@@ -1 +1 @@
-179060
+179126

.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap

@@ -1 +1 @@
-133063
+133129

.forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap

@@ -1 +1 @@
-304484
+304550

.forge-snapshots/CLCustomCurveHookTest#test_Swap_CustomCurve.snap

@@ -1 +1 @@
-149600
+149710

.forge-snapshots/CLMintBurnFeeHookTest#test_Burn.snap

@@ -1 +1 @@
-170881
+170991

.forge-snapshots/CLMintBurnFeeHookTest#test_Mint.snap

@@ -1 +1 @@
-421255
+421365

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap

@@ -1 +1 @@
-347480
+347568

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap

@@ -1 +1 @@
-162941
+163029

.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap

@@ -1 +1 @@
-238442
+238486

.forge-snapshots/CLPoolManagerTest#donateBothTokens.snap

@@ -1 +1 @@
-163218
+163306

.forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap

@@ -1 +1 @@
-108268
+108334

.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap

@@ -1 +1 @@
-112784
+112828

.forge-snapshots/CLPoolManagerTest#swap_againstLiquidity.snap

@@ -1 +1 @@
-130830
+130896

.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap

@@ -1 +1 @@
-163213
+163301

.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap

@@ -1 +1 @@
-148610
+148676

.forge-snapshots/CLPoolManagerTest#swap_simple.snap

@@ -1 +1 @@
-71289
+71333

.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap

@@ -1 +1 @@
-143194
+143282

.forge-snapshots/CLPoolManagerTest#swap_withHooks.snap

@@ -1 +1 @@
-87479
+87523

.forge-snapshots/CLPoolManagerTest#swap_withNative.snap

@@ -1 +1 @@
-71292
+71336

.forge-snapshots/VaultBytecodeSize.snap

@@ -1 +1 @@
-8131
+8388

.forge-snapshots/VaultTest#testVault_clear_successWithNonZeroExistingDelta.snap

@@ -1 +1 @@
-2876
+2898

.forge-snapshots/VaultTest#testVault_clear_successWithZeroExistingDelta.snap

@@ -1 +1 @@
-1880
+1902

src/Vault.sol

@@ -2,7 +2,7 @@
 // Copyright (C) 2024 PancakeSwap
 pragma solidity 0.8.26;
 
-import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 import {IVault, IVaultToken} from "./interfaces/IVault.sol";
 import {SettlementGuard} from "./libraries/SettlementGuard.sol";
 import {Currency, CurrencyLibrary} from "./types/Currency.sol";
@@ -12,7 +12,7 @@ import {SafeCast} from "./libraries/SafeCast.sol";
 import {VaultReserve} from "./libraries/VaultReserve.sol";
 import {VaultToken} from "./VaultToken.sol";
 
-contract Vault is IVault, VaultToken, Ownable {
+contract Vault is IVault, VaultToken, Ownable2Step {
     using SafeCast for *;
     using CurrencyLibrary for Currency;
 

src/base/PoolManagerOwnable2Step.sol

@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) 2024 PancakeSwap
+pragma solidity 0.8.26;
+
+import {IPoolManagerOwner} from "../interfaces/IPoolManagerOwner.sol";
+
+/**
+ * @notice dev This contract implements "Ownable2Step styled" poolManager ownership transfer
+ * functionality. Namely an extra acceptance step is added to the ownership transfer process.
+ * This is done to prevent accidental ownership transfer to a wrong address.
+ */
+abstract contract PoolManagerOwnable2Step is IPoolManagerOwner {
+    error NotPendingPoolManagerOwner();
+
+    event PoolManagerOwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);
+    event PoolManagerOwnershipTransferred(address indexed previousOwner, address indexed newOwner);
+
+    address private _pendingPoolManagerOwner;
+
+    modifier onlyPendingPoolManagerOwner() {
+        if (_pendingPoolManagerOwner != msg.sender) {
+            revert NotPendingPoolManagerOwner();
+        }
+
+        _;
+    }
+
+    function _setPendingPoolManagerOwner(address newPoolManagerOwner) internal {
+        _pendingPoolManagerOwner = newPoolManagerOwner;
+    }
+
+    /// @inheritdoc IPoolManagerOwner
+    function pendingPoolManagerOwner() public view override returns (address) {
+        return _pendingPoolManagerOwner;
+    }
+}

src/interfaces/IPoolManagerOwner.sol

@@ -18,6 +18,14 @@ interface IPoolManagerOwner {
 
     /// @notice transfer the ownership of pool manager to the new owner
     /// @dev used when a new PoolManagerOwner contract is created and we transfer pool manager owner to new contract
-    /// @param newOwner the address of the new owner
-    function transferPoolManagerOwnership(address newOwner) external;
+    /// @param newPoolManagerOwner the address of the new owner
+    function transferPoolManagerOwnership(address newPoolManagerOwner) external;
+
+    /// @notice accept the ownership of pool manager, only callable by the
+    /// pending pool manager owner set by latest transferPoolManagerOwnership
+    function acceptPoolManagerOwnership() external;
+
+    /// @notice get the current pending pool manager owner
+    /// @return the address of the pending pool manager owner
+    function pendingPoolManagerOwner() external view returns (address);
 }

src/pool-bin/BinPoolManagerOwner.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.26;
 
 import {IProtocolFeeController} from "../interfaces/IProtocolFeeController.sol";
 import {PausableRole} from "../base/PausableRole.sol";
+import {PoolManagerOwnable2Step} from "../base/PoolManagerOwnable2Step.sol";
 import {IBinPoolManager} from "./interfaces/IBinPoolManager.sol";
 import {IPoolManagerOwner} from "../interfaces/IPoolManagerOwner.sol";
 
@@ -20,7 +21,7 @@ interface IBinPoolManagerWithPauseOwnable is IBinPoolManager {
  * A seperate owner contract is used to handle some functionality so as to reduce the contract size
  * of PoolManager. This allow a higher optimizer run, reducing the gas cost for other poolManager functions
  */
-contract BinPoolManagerOwner is IPoolManagerOwner, PausableRole {
+contract BinPoolManagerOwner is IPoolManagerOwner, PoolManagerOwnable2Step, PausableRole {
     /// @notice Error thrown when owner set min share too small
     error MinShareTooSmall(uint256 minShare);
 
@@ -46,8 +47,16 @@ contract BinPoolManagerOwner is IPoolManagerOwner, PausableRole {
     }
 
     /// @inheritdoc IPoolManagerOwner
-    function transferPoolManagerOwnership(address newOwner) external override onlyOwner {
-        poolManager.transferOwnership(newOwner);
+    function transferPoolManagerOwnership(address newPoolManagerOwner) external override onlyOwner {
+        _setPendingPoolManagerOwner(newPoolManagerOwner);
+        emit PoolManagerOwnershipTransferStarted(address(this), newPoolManagerOwner);
+    }
+
+    /// @inheritdoc IPoolManagerOwner
+    function acceptPoolManagerOwnership() external override onlyPendingPoolManagerOwner {
+        _setPendingPoolManagerOwner(address(0));
+        poolManager.transferOwnership(msg.sender);
+        emit PoolManagerOwnershipTransferred(address(this), msg.sender);
     }
 
     /// @notice Set max bin steps for binPoolManager, see IBinPoolManager for more documentation about this function

src/pool-cl/CLPoolManagerOwner.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.26;
 
 import {IProtocolFeeController} from "../interfaces/IProtocolFeeController.sol";
 import {PausableRole} from "../base/PausableRole.sol";
+import {PoolManagerOwnable2Step} from "../base/PoolManagerOwnable2Step.sol";
 import {ICLPoolManager} from "./interfaces/ICLPoolManager.sol";
 import {IPoolManagerOwner} from "../interfaces/IPoolManagerOwner.sol";
 
@@ -20,7 +21,7 @@ interface ICLPoolManagerWithPauseOwnable is ICLPoolManager {
  * A seperate owner contract is used to handle some functionality so as to reduce the contract size
  * of PoolManager. This allow a higher optimizer run, reducing the gas cost for other poolManager functions.
  */
-contract CLPoolManagerOwner is IPoolManagerOwner, PausableRole {
+contract CLPoolManagerOwner is IPoolManagerOwner, PoolManagerOwnable2Step, PausableRole {
     ICLPoolManagerWithPauseOwnable public immutable poolManager;
 
     constructor(ICLPoolManagerWithPauseOwnable _poolManager) {
@@ -43,7 +44,16 @@ contract CLPoolManagerOwner is IPoolManagerOwner, PausableRole {
     }
 
     /// @inheritdoc IPoolManagerOwner
-    function transferPoolManagerOwnership(address newOwner) external override onlyOwner {
-        poolManager.transferOwnership(newOwner);
+    function transferPoolManagerOwnership(address newPoolManagerOwner) external override onlyOwner {
+        _setPendingPoolManagerOwner(newPoolManagerOwner);
+        emit PoolManagerOwnershipTransferStarted(address(this), newPoolManagerOwner);
+    }
+
+    /// @inheritdoc IPoolManagerOwner
+    function acceptPoolManagerOwnership() external override onlyPendingPoolManagerOwner {
+        _setPendingPoolManagerOwner(address(0));
+        poolManager.transferOwnership(msg.sender);
+
+        emit PoolManagerOwnershipTransferred(address(this), msg.sender);
     }
 }

test/pool-bin/BinPoolManagerOwner.t.sol

@@ -7,6 +7,7 @@ import {IVault} from "../../src/interfaces/IVault.sol";
 import {Vault} from "../../src/Vault.sol";
 import {BinPoolManager} from "../../src/pool-bin/BinPoolManager.sol";
 import {BinPoolManagerOwner, IBinPoolManagerWithPauseOwnable} from "../../src/pool-bin/BinPoolManagerOwner.sol";
+import {PoolManagerOwnable2Step} from "../../src/base/PoolManagerOwnable2Step.sol";
 import {Pausable} from "../../src/base/Pausable.sol";
 import {PausableRole} from "../../src/base/PausableRole.sol";
 
@@ -110,8 +111,19 @@ contract BinPoolManagerOwnerTest is Test {
         // before:
         assertEq(poolManager.owner(), address(binPoolManagerOwner));
 
-        // after:
+        // pending:
+        // it's still the original owner if new owner not accept yet
+        vm.expectEmit(true, true, true, true);
+        emit PoolManagerOwnable2Step.PoolManagerOwnershipTransferStarted(address(binPoolManagerOwner), alice);
         binPoolManagerOwner.transferPoolManagerOwnership(alice);
+        assertEq(poolManager.owner(), address(binPoolManagerOwner));
+        assertEq(binPoolManagerOwner.pendingPoolManagerOwner(), alice);
+
+        // after:
+        vm.expectEmit(true, true, true, true);
+        emit PoolManagerOwnable2Step.PoolManagerOwnershipTransferred(address(binPoolManagerOwner), alice);
+        vm.prank(alice);
+        binPoolManagerOwner.acceptPoolManagerOwnership();
         assertEq(poolManager.owner(), alice);
     }
 
@@ -122,6 +134,40 @@ contract BinPoolManagerOwnerTest is Test {
         binPoolManagerOwner.transferPoolManagerOwnership(alice);
     }
 
+    function test_TransferPoolManagerOwnership_NotPendingPoolManagerOwner() public {
+        binPoolManagerOwner.transferPoolManagerOwnership(alice);
+
+        // if it's not from alice then revert
+        vm.expectRevert(PoolManagerOwnable2Step.NotPendingPoolManagerOwner.selector);
+        binPoolManagerOwner.acceptPoolManagerOwnership();
+    }
+
+    function test_TransferPoolManagerOwnership_OverridePendingOwner() public {
+        // before:
+        assertEq(poolManager.owner(), address(binPoolManagerOwner));
+
+        // pending:
+        // it's still the original owner if new owner not accept yet
+        binPoolManagerOwner.transferPoolManagerOwnership(alice);
+        assertEq(poolManager.owner(), address(binPoolManagerOwner));
+        assertEq(binPoolManagerOwner.pendingPoolManagerOwner(), alice);
+
+        // override pending owner
+        binPoolManagerOwner.transferPoolManagerOwnership(makeAddr("bob"));
+        assertEq(poolManager.owner(), address(binPoolManagerOwner));
+        assertEq(binPoolManagerOwner.pendingPoolManagerOwner(), makeAddr("bob"));
+
+        // alice no longer the valid pending owner
+        vm.expectRevert(PoolManagerOwnable2Step.NotPendingPoolManagerOwner.selector);
+        vm.prank(alice);
+        binPoolManagerOwner.acceptPoolManagerOwnership();
+
+        // bob is the new owner
+        vm.prank(makeAddr("bob"));
+        binPoolManagerOwner.acceptPoolManagerOwnership();
+        assertEq(poolManager.owner(), makeAddr("bob"));
+    }
+
     function test_SetMaxBinStep_OnlyOwner() public {
         // before
         assertEq(poolManager.maxBinStep(), 100);