From f5458fc266b3773c5ffa7aef2214df09b6677eef Mon Sep 17 00:00:00 2001 From: Victoria Zotova Date: Mon, 15 Jul 2024 11:11:25 -0400 Subject: [PATCH 1/4] BqETHSubscription: adopter set by special role --- .../subscription/BqETHSubscription.sol | 22 +++++-- tests/test_bqeth_subscription.py | 58 ++++++++++++++++--- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/contracts/contracts/coordination/subscription/BqETHSubscription.sol b/contracts/contracts/coordination/subscription/BqETHSubscription.sol index aa0bcd8f1..39c7246f4 100644 --- a/contracts/contracts/coordination/subscription/BqETHSubscription.sol +++ b/contracts/contracts/coordination/subscription/BqETHSubscription.sol @@ -26,7 +26,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable GlobalAllowList public immutable accessController; IERC20 public immutable feeToken; - address public immutable adopter; + address public immutable adopterSetter; uint256 public immutable initialBaseFeeRate; uint256 public immutable baseFeeRateIncrease; @@ -35,8 +35,9 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable uint32 public activeRitualId; mapping(uint256 periodNumber => Billing billing) public billingInfo; + address public adopter; - uint256[20] private gap; + uint256[19] private gap; /** * @notice Emitted when a subscription is spent @@ -79,7 +80,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable * @param _coordinator The address of the coordinator contract * @param _accessController The address of the global allow list * @param _feeToken The address of the fee token contract - * @param _adopter The address of the adopter + * @param _adopterSetter The address of the adopter * @param _initialBaseFeeRate Fee rate per node per second * @param _baseFeeRateIncrease Increase of base fee rate per each period (fraction of INCREASE_BASE) * @param _encryptorFeeRate Fee rate per encryptor per second @@ -92,7 +93,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable Coordinator _coordinator, GlobalAllowList _accessController, IERC20 _feeToken, - address _adopter, + address _adopterSetter, uint256 _initialBaseFeeRate, uint256 _baseFeeRateIncrease, uint256 _encryptorFeeRate, @@ -109,7 +110,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable ) { require(address(_feeToken) != address(0), "Fee token cannot be the zero address"); - require(_adopter != address(0), "Adopter cannot be the zero address"); + require(_adopterSetter != address(0), "Adopter setter cannot be the zero address"); require( address(_accessController) != address(0), "Access controller cannot be the zero address" @@ -119,7 +120,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable "Base fee rate increase must be fraction of INCREASE_BASE" ); feeToken = _feeToken; - adopter = _adopter; + adopterSetter = _adopterSetter; initialBaseFeeRate = _initialBaseFeeRate; baseFeeRateIncrease = _baseFeeRateIncrease; encryptorFeeRate = _encryptorFeeRate; @@ -152,6 +153,15 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable __Ownable_init(_treasury); } + function setAdopter(address _adopter) external { + require(msg.sender == adopterSetter, "Only authority can set adopter"); + require( + adopter == address(0) && _adopter != address(0), + "Adopter can be set only once with not zero address" + ); + adopter = _adopter; + } + function baseFees() public view returns (uint256) { uint256 currentPeriodNumber = getCurrentPeriodNumber(); return baseFees(currentPeriodNumber); diff --git a/tests/test_bqeth_subscription.py b/tests/test_bqeth_subscription.py index 44f0e2999..815b5157b 100644 --- a/tests/test_bqeth_subscription.py +++ b/tests/test_bqeth_subscription.py @@ -19,6 +19,7 @@ import ape import pytest +from ape.utils import ZERO_ADDRESS from eth_account.messages import encode_defunct from web3 import Web3 @@ -72,6 +73,11 @@ def adopter(accounts): return accounts[2] +@pytest.fixture(scope="module") +def adopter_setter(accounts): + return accounts[3] + + @pytest.fixture() def erc20(project, adopter): token = project.TestToken.deploy(ERC20_SUPPLY, sender=adopter) @@ -94,13 +100,13 @@ def global_allow_list(project, creator, coordinator): @pytest.fixture() def subscription( - project, creator, coordinator, global_allow_list, erc20, adopter, treasury, oz_dependency + project, creator, coordinator, global_allow_list, erc20, adopter_setter, treasury, oz_dependency ): contract = project.BqETHSubscription.deploy( coordinator.address, global_allow_list.address, erc20.address, - adopter, + adopter_setter, BASE_FEE_RATE, BASE_FEE_RATE_INCREASE * 100, ENCRYPTORS_FEE_RATE, @@ -124,10 +130,22 @@ def subscription( return proxy_contract +def test_adopter_setter(subscription, adopter_setter, adopter): + with ape.reverts("Only authority can set adopter"): + subscription.setAdopter(adopter, sender=adopter) + with ape.reverts("Adopter can be set only once with not zero address"): + subscription.setAdopter(ZERO_ADDRESS, sender=adopter_setter) + subscription.setAdopter(adopter, sender=adopter_setter) + assert subscription.adopter() == adopter + with ape.reverts("Adopter can be set only once with not zero address"): + subscription.setAdopter(adopter_setter, sender=adopter_setter) + + def test_pay_subscription( - erc20, subscription, coordinator, global_allow_list, adopter, treasury, chain + erc20, subscription, coordinator, global_allow_list, adopter, adopter_setter, treasury, chain ): erc20.approve(subscription.address, ERC20_SUPPLY, sender=adopter) + subscription.setAdopter(adopter, sender=adopter_setter) # First payment balance_before = erc20.balanceOf(adopter) @@ -250,7 +268,7 @@ def test_pay_subscription( def test_pay_encryptor_slots( - erc20, subscription, coordinator, global_allow_list, adopter, treasury, chain + erc20, subscription, coordinator, global_allow_list, adopter, adopter_setter, treasury, chain ): encryptor_slots = 10 assert ( @@ -259,6 +277,7 @@ def test_pay_encryptor_slots( ) erc20.approve(subscription.address, ERC20_SUPPLY, sender=adopter) + subscription.setAdopter(adopter, sender=adopter_setter) with ape.reverts("Current billing period must be paid"): subscription.payForEncryptorSlots(encryptor_slots, sender=adopter) @@ -333,8 +352,9 @@ def test_pay_encryptor_slots( subscription.payForEncryptorSlots(encryptor_slots, sender=adopter) -def test_withdraw(erc20, subscription, adopter, treasury, global_allow_list): +def test_withdraw(erc20, subscription, adopter, adopter_setter, treasury, global_allow_list): erc20.approve(subscription.address, ERC20_SUPPLY, sender=adopter) + subscription.setAdopter(adopter, sender=adopter_setter) with ape.reverts(): subscription.withdrawToTreasury(1, sender=adopter) @@ -357,10 +377,11 @@ def test_withdraw(erc20, subscription, adopter, treasury, global_allow_list): def test_process_ritual_payment( - erc20, subscription, coordinator, global_allow_list, adopter, treasury + erc20, subscription, coordinator, global_allow_list, adopter, adopter_setter, treasury ): ritual_id = 7 number_of_providers = 6 + subscription.setAdopter(adopter, sender=adopter_setter) with ape.reverts("Only the Coordinator can call this method"): subscription.processRitualPayment( @@ -460,7 +481,7 @@ def test_process_ritual_payment( def test_process_ritual_extending( - erc20, subscription, coordinator, adopter, global_allow_list, treasury + erc20, subscription, coordinator, adopter, adopter_setter, global_allow_list, treasury ): ritual_id = 6 number_of_providers = 7 @@ -475,6 +496,7 @@ def test_process_ritual_extending( ) erc20.approve(subscription.address, ERC20_SUPPLY, sender=adopter) + subscription.setAdopter(adopter, sender=adopter_setter) subscription.payForSubscription(0, sender=adopter) coordinator.setRitual( ritual_id, RitualState.ACTIVE, 0, global_allow_list.address, sender=treasury @@ -535,7 +557,15 @@ def test_process_ritual_extending( def test_before_set_authorization( - erc20, subscription, coordinator, adopter, global_allow_list, treasury, creator, chain + erc20, + subscription, + coordinator, + adopter, + adopter_setter, + global_allow_list, + treasury, + creator, + chain, ): ritual_id = 6 number_of_providers = 7 @@ -547,6 +577,7 @@ def test_before_set_authorization( global_allow_list.authorize(0, [creator], sender=adopter) erc20.approve(subscription.address, ERC20_SUPPLY, sender=adopter) + subscription.setAdopter(adopter, sender=adopter_setter) subscription.payForSubscription(0, sender=adopter) coordinator.setRitual( ritual_id, RitualState.ACTIVE, 0, global_allow_list.address, sender=treasury @@ -592,7 +623,15 @@ def test_before_set_authorization( def test_before_is_authorized( - erc20, subscription, coordinator, adopter, global_allow_list, treasury, creator, chain + erc20, + subscription, + coordinator, + adopter, + adopter_setter, + global_allow_list, + treasury, + creator, + chain, ): ritual_id = 6 @@ -610,6 +649,7 @@ def test_before_is_authorized( global_allow_list.isAuthorized(0, bytes(signature), bytes(data)) erc20.approve(subscription.address, ERC20_SUPPLY, sender=adopter) + subscription.setAdopter(adopter, sender=adopter_setter) subscription.payForSubscription(1, sender=adopter) coordinator.setRitual( ritual_id, RitualState.ACTIVE, 0, global_allow_list.address, sender=treasury From 71a625d6c50ecce89d5f1f1e4f9a879f276da4c1 Mon Sep 17 00:00:00 2001 From: Victoria Date: Mon, 15 Jul 2024 18:11:20 +0200 Subject: [PATCH 2/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David Núñez --- .../coordination/subscription/BqETHSubscription.sol | 6 +++--- tests/test_bqeth_subscription.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/contracts/coordination/subscription/BqETHSubscription.sol b/contracts/contracts/coordination/subscription/BqETHSubscription.sol index 39c7246f4..5430cb8d7 100644 --- a/contracts/contracts/coordination/subscription/BqETHSubscription.sol +++ b/contracts/contracts/coordination/subscription/BqETHSubscription.sol @@ -37,7 +37,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable mapping(uint256 periodNumber => Billing billing) public billingInfo; address public adopter; - uint256[19] private gap; + uint256[20] private gap; /** * @notice Emitted when a subscription is spent @@ -80,7 +80,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable * @param _coordinator The address of the coordinator contract * @param _accessController The address of the global allow list * @param _feeToken The address of the fee token contract - * @param _adopterSetter The address of the adopter + * @param _adopterSetter Address that can set the adopter address * @param _initialBaseFeeRate Fee rate per node per second * @param _baseFeeRateIncrease Increase of base fee rate per each period (fraction of INCREASE_BASE) * @param _encryptorFeeRate Fee rate per encryptor per second @@ -154,7 +154,7 @@ contract BqETHSubscription is EncryptorSlotsSubscription, Initializable, Ownable } function setAdopter(address _adopter) external { - require(msg.sender == adopterSetter, "Only authority can set adopter"); + require(msg.sender == adopterSetter, "Only adopter setter can set adopter"); require( adopter == address(0) && _adopter != address(0), "Adopter can be set only once with not zero address" diff --git a/tests/test_bqeth_subscription.py b/tests/test_bqeth_subscription.py index 815b5157b..352a5e1c5 100644 --- a/tests/test_bqeth_subscription.py +++ b/tests/test_bqeth_subscription.py @@ -131,7 +131,7 @@ def subscription( def test_adopter_setter(subscription, adopter_setter, adopter): - with ape.reverts("Only authority can set adopter"): + with ape.reverts("Only adopter setter can set adopter"): subscription.setAdopter(adopter, sender=adopter) with ape.reverts("Adopter can be set only once with not zero address"): subscription.setAdopter(ZERO_ADDRESS, sender=adopter_setter) From 3f65b2baffff5bb445f4e267d03367be99fcb995 Mon Sep 17 00:00:00 2001 From: Victoria Zotova Date: Mon, 15 Jul 2024 12:49:06 -0400 Subject: [PATCH 3/4] Fix for #292 --- .../subscription/EncryptorSlotsSubscription.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/contracts/coordination/subscription/EncryptorSlotsSubscription.sol b/contracts/contracts/coordination/subscription/EncryptorSlotsSubscription.sol index 8522fd203..1b4c5a17a 100644 --- a/contracts/contracts/coordination/subscription/EncryptorSlotsSubscription.sol +++ b/contracts/contracts/coordination/subscription/EncryptorSlotsSubscription.sol @@ -88,7 +88,11 @@ abstract contract EncryptorSlotsSubscription is AbstractSubscription { usedEncryptorSlots += addresses.length; require(usedEncryptorSlots <= encryptorSlots, "Encryptors slots filled up"); } else { - usedEncryptorSlots -= addresses.length; + if (usedEncryptorSlots >= addresses.length) { + usedEncryptorSlots -= addresses.length; + } else { + usedEncryptorSlots = 0; + } } } From d2ae1da82206d71f313ea90660e34d31b5b67468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=BA=C3=B1ez?= Date: Tue, 16 Jul 2024 10:41:49 +0200 Subject: [PATCH 4/4] Remove MAX_AUTH_ACTIONS restriction --- contracts/contracts/coordination/GlobalAllowList.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contracts/contracts/coordination/GlobalAllowList.sol b/contracts/contracts/coordination/GlobalAllowList.sol index 656073be2..b49fa60d3 100644 --- a/contracts/contracts/coordination/GlobalAllowList.sol +++ b/contracts/contracts/coordination/GlobalAllowList.sol @@ -22,8 +22,6 @@ contract GlobalAllowList is IEncryptionAuthorizer { mapping(uint32 => uint256) public authActions; - uint32 public constant MAX_AUTH_ACTIONS = 100; - /** * @notice Emitted when an address authorization is set * @param ritualId The ID of the ritual @@ -153,8 +151,6 @@ contract GlobalAllowList is IEncryptionAuthorizer { function setAuthorizations(uint32 ritualId, address[] calldata addresses, bool value) internal { require(coordinator.isRitualActive(ritualId), "Only active rituals can set authorizations"); - require(addresses.length <= MAX_AUTH_ACTIONS, "Too many addresses"); - _beforeSetAuthorization(ritualId, addresses, value); for (uint256 i = 0; i < addresses.length; i++) { bytes32 lookupKey = LookupKey.lookupKey(ritualId, addresses[i]);