From 6949b223befc2802d83c3d46682ccaff898b1e41 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 13 Feb 2024 11:27:29 -0500 Subject: [PATCH] Make the V3FactoryOwner payout amount settable by admin The setter also enforces that the payout amount is non-zero, and emits an event. The same applies in the constructor. The constructor also now emits an AdminSet event for initialization of the original admin. --- src/V3FactoryOwner.sol | 31 +++++-- test/UniStaker.integration.t.sol | 2 +- test/V3FactoryOwner.t.sol | 144 ++++++++++++++++++++++++++----- 3 files changed, 149 insertions(+), 28 deletions(-) diff --git a/src/V3FactoryOwner.sol b/src/V3FactoryOwner.sol index 062fd3b..432ffc1 100644 --- a/src/V3FactoryOwner.sol +++ b/src/V3FactoryOwner.sol @@ -47,12 +47,18 @@ contract V3FactoryOwner { /// @notice Emitted when the existing admin designates a new address as the admin. event AdminSet(address indexed oldAmin, address indexed newAdmin); + /// @notice Emitted when the admin updates the payout amount. + event PayoutAmountSet(uint256 indexed oldPayoutAmount, uint256 indexed newPayoutAmount); + /// @notice Thrown when an unauthorized account calls a privileged function. error V3FactoryOwner__Unauthorized(); /// @notice Thrown if the proposed admin is the zero address. error V3FactoryOwner__InvalidAddress(); + /// @notice Thrown if the proposed payout amount is zero. + error V3FactoryOwner__InvalidPayoutAmount(); + /// @notice Thrown when the fees collected from a pool are less than the caller expects. error V3FactoryOwner__InsufficientFeesCollected(); @@ -63,7 +69,7 @@ contract V3FactoryOwner { IERC20 public immutable PAYOUT_TOKEN; /// @notice The raw amount of the payout token which is paid by a user when claiming pool fees. - uint256 public immutable PAYOUT_AMOUNT; + uint256 public payoutAmount; /// @notice The contract that receives the payout and is notified via method call, when pool fees /// are claimed. @@ -76,7 +82,8 @@ contract V3FactoryOwner { /// @param _admin The initial admin address for this deployment. Cannot be zero address. /// @param _factory The v3 factory instance for which this deployment will serve as owner. /// @param _payoutToken The ERC-20 token in which payouts will be denominated. - /// @param _payoutAmount The raw amount of the payout token required to claim fees from a pool. + /// @param _payoutAmount The initial raw amount of the payout token required to claim fees from + /// a pool. /// @param _rewardReceiver The contract that will receive the payout when fees are claimed. constructor( address _admin, @@ -86,11 +93,16 @@ contract V3FactoryOwner { INotifiableRewardReceiver _rewardReceiver ) { if (_admin == address(0)) revert V3FactoryOwner__InvalidAddress(); + if (_payoutAmount == 0) revert V3FactoryOwner__InvalidPayoutAmount(); + admin = _admin; FACTORY = _factory; PAYOUT_TOKEN = _payoutToken; - PAYOUT_AMOUNT = _payoutAmount; + payoutAmount = _payoutAmount; REWARD_RECEIVER = _rewardReceiver; + + emit AdminSet(address(0), _admin); + emit PayoutAmountSet(0, _payoutAmount); } /// @notice Pass the admin role to a new address. Must be called by the existing admin. @@ -102,6 +114,15 @@ contract V3FactoryOwner { admin = _newAdmin; } + /// @notice Update the payout amount to a new value. Must be called by admin. + /// @param _newPayoutAmount The value that will be the new payout amount. + function setPayoutAmount(uint256 _newPayoutAmount) external { + _revertIfNotAdmin(); + if (_newPayoutAmount == 0) revert V3FactoryOwner__InvalidPayoutAmount(); + emit PayoutAmountSet(payoutAmount, _newPayoutAmount); + payoutAmount = _newPayoutAmount; + } + /// @notice Passthrough method that enables a fee amount on the factory. Must be called by the /// admin. /// @param _fee The fee param to forward to the factory. @@ -163,8 +184,8 @@ contract V3FactoryOwner { uint128 _amount0Requested, uint128 _amount1Requested ) external returns (uint128, uint128) { - PAYOUT_TOKEN.safeTransferFrom(msg.sender, address(REWARD_RECEIVER), PAYOUT_AMOUNT); - REWARD_RECEIVER.notifyRewardAmount(PAYOUT_AMOUNT); + PAYOUT_TOKEN.safeTransferFrom(msg.sender, address(REWARD_RECEIVER), payoutAmount); + REWARD_RECEIVER.notifyRewardAmount(payoutAmount); (uint128 _amount0, uint128 _amount1) = _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested); diff --git a/test/UniStaker.integration.t.sol b/test/UniStaker.integration.t.sol index 83d9121..5988ae6 100644 --- a/test/UniStaker.integration.t.sol +++ b/test/UniStaker.integration.t.sol @@ -21,7 +21,7 @@ contract DeployScriptTest is Test, DeployInput { assertEq(v3FactoryOwner.admin(), UNISWAP_GOVERNOR_TIMELOCK); assertEq(address(v3FactoryOwner.FACTORY()), address(UNISWAP_V3_FACTORY_ADDRESS)); assertEq(address(v3FactoryOwner.PAYOUT_TOKEN()), PAYOUT_TOKEN_ADDRESS); - assertEq(v3FactoryOwner.PAYOUT_AMOUNT(), PAYOUT_AMOUNT); + assertEq(v3FactoryOwner.payoutAmount(), PAYOUT_AMOUNT); assertEq(address(v3FactoryOwner.REWARD_RECEIVER()), address(uniStaker)); assertEq(address(uniStaker.REWARD_TOKEN()), PAYOUT_TOKEN_ADDRESS); diff --git a/test/V3FactoryOwner.t.sol b/test/V3FactoryOwner.t.sol index 8747f23..2db6623 100644 --- a/test/V3FactoryOwner.t.sol +++ b/test/V3FactoryOwner.t.sol @@ -21,15 +21,6 @@ contract V3FactoryOwnerTest is Test { MockUniswapV3Pool pool; MockUniswapV3Factory factory; - event AdminSet(address indexed oldAmin, address indexed newAdmin); - event FeesClaimed( - address indexed pool, - address indexed caller, - address indexed recipient, - uint256 amount0, - uint256 amount1 - ); - function setUp() public { vm.label(admin, "Admin"); @@ -49,6 +40,7 @@ contract V3FactoryOwnerTest is Test { // In order to fuzz over the payout amount, we require each test to call this method to deploy // the factory owner before doing anything else. function _deployFactoryOwnerWithPayoutAmount(uint256 _payoutAmount) public { + vm.assume(_payoutAmount != 0); factoryOwner = new V3FactoryOwner(admin, factory, payoutToken, _payoutAmount, rewardReceiver); vm.label(address(factoryOwner), "Factory Owner"); } @@ -61,7 +53,7 @@ contract Constructor is V3FactoryOwnerTest { assertEq(factoryOwner.admin(), admin); assertEq(address(factoryOwner.FACTORY()), address(factory)); assertEq(address(factoryOwner.PAYOUT_TOKEN()), address(payoutToken)); - assertEq(factoryOwner.PAYOUT_AMOUNT(), _payoutAmount); + assertEq(factoryOwner.payoutAmount(), _payoutAmount); assertEq(address(factoryOwner.REWARD_RECEIVER()), address(rewardReceiver)); } @@ -72,7 +64,7 @@ contract Constructor is V3FactoryOwnerTest { uint256 _payoutAmount, address _rewardReceiver ) public { - vm.assume(_admin != address(0)); + vm.assume(_admin != address(0) && _payoutAmount != 0); V3FactoryOwner _factoryOwner = new V3FactoryOwner( _admin, IUniswapV3FactoryOwnerActions(_factory), @@ -83,16 +75,57 @@ contract Constructor is V3FactoryOwnerTest { assertEq(_factoryOwner.admin(), _admin); assertEq(address(_factoryOwner.FACTORY()), address(_factory)); assertEq(address(_factoryOwner.PAYOUT_TOKEN()), address(_payoutToken)); - assertEq(_factoryOwner.PAYOUT_AMOUNT(), _payoutAmount); + assertEq(_factoryOwner.payoutAmount(), _payoutAmount); assertEq(address(_factoryOwner.REWARD_RECEIVER()), _rewardReceiver); } + function testFuzz_EmitsAdminSetEvent( + address _admin, + address _factory, + address _payoutToken, + uint256 _payoutAmount, + address _rewardReceiver + ) public { + vm.assume(_admin != address(0) && _payoutAmount != 0); + + vm.expectEmit(); + emit V3FactoryOwner.AdminSet(address(0), _admin); + new V3FactoryOwner( + _admin, + IUniswapV3FactoryOwnerActions(_factory), + IERC20(_payoutToken), + _payoutAmount, + INotifiableRewardReceiver(_rewardReceiver) + ); + } + + function testFuzz_EmitsPayoutSetEvent( + address _admin, + address _factory, + address _payoutToken, + uint256 _payoutAmount, + address _rewardReceiver + ) public { + vm.assume(_admin != address(0) && _payoutAmount != 0); + + vm.expectEmit(); + emit V3FactoryOwner.PayoutAmountSet(0, _payoutAmount); + new V3FactoryOwner( + _admin, + IUniswapV3FactoryOwnerActions(_factory), + IERC20(_payoutToken), + _payoutAmount, + INotifiableRewardReceiver(_rewardReceiver) + ); + } + function testFuzz_RevertIf_TheAdminIsAddressZero( address _factory, address _payoutToken, uint256 _payoutAmount, address _rewardReceiver ) public { + vm.assume(_payoutAmount != 0); vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InvalidAddress.selector); new V3FactoryOwner( address(0), @@ -102,12 +135,30 @@ contract Constructor is V3FactoryOwnerTest { INotifiableRewardReceiver(_rewardReceiver) ); } + + function testFuzz_RevertIf_ThePayoutAmountIsZero( + address _admin, + address _factory, + address _payoutToken, + address _rewardReceiver + ) public { + vm.assume(_admin != address(0)); + + vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InvalidPayoutAmount.selector); + new V3FactoryOwner( + _admin, + IUniswapV3FactoryOwnerActions(_factory), + IERC20(_payoutToken), + 0, + INotifiableRewardReceiver(_rewardReceiver) + ); + } } contract SetAdmin is V3FactoryOwnerTest { function testFuzz_UpdatesTheAdminWhenCalledByTheCurrentAdmin(address _newAdmin) public { vm.assume(_newAdmin != address(0)); - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.prank(admin); factoryOwner.setAdmin(_newAdmin); @@ -117,18 +168,18 @@ contract SetAdmin is V3FactoryOwnerTest { function testFuzz_EmitsAnEventWhenUpdatingTheAdmin(address _newAdmin) public { vm.assume(_newAdmin != address(0)); - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.expectEmit(); vm.prank(admin); - emit AdminSet(admin, _newAdmin); + emit V3FactoryOwner.AdminSet(admin, _newAdmin); factoryOwner.setAdmin(_newAdmin); } function testFuzz_RevertIf_TheCallerIsNotTheCurrentAdmin(address _notAdmin, address _newAdmin) public { - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.assume(_notAdmin != admin); @@ -138,7 +189,7 @@ contract SetAdmin is V3FactoryOwnerTest { } function test_RevertIf_TheNewAdminIsAddressZero() public { - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InvalidAddress.selector); vm.prank(admin); @@ -146,12 +197,61 @@ contract SetAdmin is V3FactoryOwnerTest { } } +contract SetPayoutAmount is V3FactoryOwnerTest { + function testFuzz_UpdatesThePayoutAmountWhenCalledByAdmin( + uint256 _initialPayoutAmount, + uint256 _newPayoutAmount + ) public { + vm.assume(_newPayoutAmount != 0); + _deployFactoryOwnerWithPayoutAmount(_initialPayoutAmount); + + vm.prank(admin); + factoryOwner.setPayoutAmount(_newPayoutAmount); + + assertEq(factoryOwner.payoutAmount(), _newPayoutAmount); + } + + function testFuzz_EmitsAnEventWhenUpdatingThePayoutAmount( + uint256 _initialPayoutAmount, + uint256 _newPayoutAmount + ) public { + vm.assume(_newPayoutAmount != 0); + _deployFactoryOwnerWithPayoutAmount(_initialPayoutAmount); + + vm.expectEmit(); + vm.prank(admin); + emit V3FactoryOwner.PayoutAmountSet(_initialPayoutAmount, _newPayoutAmount); + factoryOwner.setPayoutAmount(_newPayoutAmount); + } + + function testFuzz_RevertIf_TheCallerIsNotAdmin( + uint256 _initialPayoutAmount, + uint256 _newPayoutAmount, + address _notAdmin + ) public { + vm.assume(_notAdmin != admin && _newPayoutAmount != 0); + _deployFactoryOwnerWithPayoutAmount(_initialPayoutAmount); + + vm.expectRevert(V3FactoryOwner.V3FactoryOwner__Unauthorized.selector); + vm.prank(_notAdmin); + factoryOwner.setPayoutAmount(_newPayoutAmount); + } + + function testFuzz_RevertIf_TheNewPayoutAmountIsZero(uint256 _initialPayoutAmount) public { + _deployFactoryOwnerWithPayoutAmount(_initialPayoutAmount); + + vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InvalidPayoutAmount.selector); + vm.prank(admin); + factoryOwner.setPayoutAmount(0); + } +} + contract EnableFeeAmount is V3FactoryOwnerTest { function testFuzz_ForwardsParametersToTheEnableFeeAmountMethodOnTheFactory( uint24 _fee, int24 _tickSpacing ) public { - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.prank(admin); factoryOwner.enableFeeAmount(_fee, _tickSpacing); @@ -165,7 +265,7 @@ contract EnableFeeAmount is V3FactoryOwnerTest { uint24 _fee, int24 _tickSpacing ) public { - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.assume(_notAdmin != admin); vm.expectRevert(V3FactoryOwner.V3FactoryOwner__Unauthorized.selector); @@ -179,7 +279,7 @@ contract SetFeeProtocol is V3FactoryOwnerTest { uint8 _feeProtocol0, uint8 _feeProtocol1 ) public { - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.prank(admin); factoryOwner.setFeeProtocol(pool, _feeProtocol0, _feeProtocol1); @@ -193,7 +293,7 @@ contract SetFeeProtocol is V3FactoryOwnerTest { uint8 _feeProtocol0, uint8 _feeProtocol1 ) public { - _deployFactoryOwnerWithPayoutAmount(0); + _deployFactoryOwnerWithPayoutAmount(1); vm.assume(_notAdmin != admin); vm.expectRevert(V3FactoryOwner.V3FactoryOwner__Unauthorized.selector); @@ -283,7 +383,7 @@ contract ClaimFees is V3FactoryOwnerTest { vm.startPrank(_caller); payoutToken.approve(address(factoryOwner), _payoutAmount); vm.expectEmit(); - emit FeesClaimed(address(pool), _caller, _recipient, _amount0, _amount1); + emit V3FactoryOwner.FeesClaimed(address(pool), _caller, _recipient, _amount0, _amount1); factoryOwner.claimFees(pool, _recipient, _amount0, _amount1); vm.stopPrank(); }