From ae7610b0ab61191ff8d0838b5232b9ebcc99557a Mon Sep 17 00:00:00 2001 From: Dev Kalra Date: Thu, 30 Nov 2023 16:29:35 +0530 Subject: [PATCH] [entropy] governance (#1154) * governance * correct comment * add upgradability * remove unused lib * update governance and update logic * change specifiers * fix tests * add tests * rebase bug fixes * change error name * fix tests * no need of UUPSProxy * minimise the amount of code * separate file for authorized tests * add comment * correct comment --- .../contracts/contracts/entropy/Entropy.sol | 13 +- .../contracts/entropy/EntropyGovernance.sol | 72 ++++++++++ .../contracts/entropy/EntropyState.sol | 2 + .../contracts/entropy/EntropyUpgradable.sol | 99 ++++++++++++++ .../contracts/forge-test/Entropy.t.sol | 18 ++- .../forge-test/EntropyAuthorized.t.sol | 123 ++++++++++++++++++ .../forge-test/EntropyGasBenchmark.t.sol | 21 ++- .../EntropyDifferentMagic.t.sol | 37 ++++++ .../entropy_sdk/solidity/EntropyErrors.sol | 4 + 9 files changed, 379 insertions(+), 10 deletions(-) create mode 100644 target_chains/ethereum/contracts/contracts/entropy/EntropyGovernance.sol create mode 100644 target_chains/ethereum/contracts/contracts/entropy/EntropyUpgradable.sol create mode 100644 target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol create mode 100644 target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol index 11a7c00c..e27dbb0b 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol @@ -73,13 +73,14 @@ import "./EntropyState.sol"; // protocol prior to the random number being revealed (i.e., prior to step (6) above). Integrators should ensure that // the user is always incentivized to reveal their random number, and that the protocol has an escape hatch for // cases where the user chooses not to reveal. -contract Entropy is IEntropy, EntropyState { - // TODO: Use an upgradeable proxy - constructor( +abstract contract Entropy is IEntropy, EntropyState { + function _initialize( + address admin, uint128 pythFeeInWei, address defaultProvider, bool prefillRequestStorage - ) { + ) internal { + _state.admin = admin; _state.accruedPythFeesInWei = 0; _state.pythFeeInWei = pythFeeInWei; _state.defaultProvider = defaultProvider; @@ -305,6 +306,10 @@ contract Entropy is IEntropy, EntropyState { return _state.providers[provider].feeInWei + _state.pythFeeInWei; } + function getPythFee() public view returns (uint128 feeAmount) { + return _state.pythFeeInWei; + } + function getAccruedPythFees() public view diff --git a/target_chains/ethereum/contracts/contracts/entropy/EntropyGovernance.sol b/target_chains/ethereum/contracts/contracts/entropy/EntropyGovernance.sol new file mode 100644 index 00000000..3d5d2725 --- /dev/null +++ b/target_chains/ethereum/contracts/contracts/entropy/EntropyGovernance.sol @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.0; + +import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol"; + +import "./EntropyState.sol"; + +/** + * @dev `Governance` defines a means to enacting changes to the Entropy contract. + */ +abstract contract EntropyGovernance is EntropyState { + event AdminSet(address oldAdmin, address newAdmin); + event PythFeeSet(uint oldPythFee, uint newPythFee); + event DefaultProviderSet( + address oldDefaultProvider, + address newDefaultProvider + ); + + function getAdmin() external view returns (address) { + return _state.admin; + } + + /** + * @dev Set the admin of the contract. + * + * Calls {_authoriseAdminAction}. + * + * Emits an {AdminSet} event. + */ + function setAdmin(address newAdmin) external { + _authoriseAdminAction(); + + address oldAdmin = _state.admin; + _state.admin = newAdmin; + + emit AdminSet(oldAdmin, newAdmin); + } + + /** + * @dev Set the Pyth fee in Wei + * + * Calls {_authoriseAdminAction}. + * + * Emits an {PythFeeSet} event. + */ + function setPythFee(uint128 newPythFee) external { + _authoriseAdminAction(); + + uint oldPythFee = _state.pythFeeInWei; + _state.pythFeeInWei = newPythFee; + + emit PythFeeSet(oldPythFee, newPythFee); + } + + /** + * @dev Set the default provider of the contract + * + * Calls {_authoriseAdminAction}. + * + * Emits an {DefaultProviderSet} event. + */ + function setDefaultProvider(address newDefaultProvider) external { + _authoriseAdminAction(); + + address oldDefaultProvider = _state.defaultProvider; + _state.defaultProvider = newDefaultProvider; + + emit DefaultProviderSet(oldDefaultProvider, newDefaultProvider); + } + + function _authoriseAdminAction() internal virtual; +} diff --git a/target_chains/ethereum/contracts/contracts/entropy/EntropyState.sol b/target_chains/ethereum/contracts/contracts/entropy/EntropyState.sol index f0fb4f57..6855b674 100644 --- a/target_chains/ethereum/contracts/contracts/entropy/EntropyState.sol +++ b/target_chains/ethereum/contracts/contracts/entropy/EntropyState.sol @@ -6,6 +6,8 @@ import "@pythnetwork/entropy-sdk-solidity/EntropyStructs.sol"; contract EntropyInternalStructs { struct State { + // Admin has the rights to update pyth configs + address admin; // Fee charged by the pyth protocol in wei. uint128 pythFeeInWei; // Total quantity of fees (in wei) earned by the pyth protocol that are currently stored in the contract. diff --git a/target_chains/ethereum/contracts/contracts/entropy/EntropyUpgradable.sol b/target_chains/ethereum/contracts/contracts/entropy/EntropyUpgradable.sol new file mode 100644 index 00000000..71eea6f1 --- /dev/null +++ b/target_chains/ethereum/contracts/contracts/entropy/EntropyUpgradable.sol @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol"; + +import "./EntropyGovernance.sol"; +import "./Entropy.sol"; + +contract EntropyUpgradable is + Initializable, + OwnableUpgradeable, + UUPSUpgradeable, + Entropy, + EntropyGovernance +{ + event ContractUpgraded( + address oldImplementation, + address newImplementation + ); + + // The contract will have an owner and an admin + // The owner will have all the power over it. + // The admin can set some config parameters only. + function initialize( + address owner, + address admin, + uint128 pythFeeInWei, + address defaultProvider, + bool prefillRequestStorage + ) public initializer { + __Ownable_init(); + __UUPSUpgradeable_init(); + + Entropy._initialize( + admin, + pythFeeInWei, + defaultProvider, + prefillRequestStorage + ); + + // We need to transfer the ownership from deployer to the new owner + transferOwnership(owner); + } + + /// Ensures the contract cannot be uninitialized and taken over. + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() initializer {} + + // Only allow the owner to upgrade the proxy to a new implementation. + function _authorizeUpgrade(address) internal override onlyOwner {} + + // There are some actions which both and admin and owner can perform + function _authoriseAdminAction() internal view override { + if (msg.sender != owner() && msg.sender != _state.admin) + revert EntropyErrors.Unauthorized(); + } + + // We have not overridden these methods in Pyth contracts implementation. + // But we are overriding them here because there was no owner before and + // `_authorizeUpgrade` would cause a revert for these. Now we have an owner, and + // because we want to test for the magic. We are overriding these methods. + function upgradeTo(address newImplementation) external override onlyProxy { + address oldImplementation = _getImplementation(); + _authorizeUpgrade(newImplementation); + _upgradeToAndCallUUPS(newImplementation, new bytes(0), false); + + magicCheck(); + + emit ContractUpgraded(oldImplementation, _getImplementation()); + } + + function upgradeToAndCall( + address newImplementation, + bytes memory data + ) external payable override onlyProxy { + address oldImplementation = _getImplementation(); + _authorizeUpgrade(newImplementation); + _upgradeToAndCallUUPS(newImplementation, data, true); + + magicCheck(); + + emit ContractUpgraded(oldImplementation, _getImplementation()); + } + + function magicCheck() internal view { + // Calling a method using `this.` will cause a contract call that will use + // the new contract. This call will fail if the method does not exists or the magic + // is different. + if (this.entropyUpgradableMagic() != 0x66697265) + revert EntropyErrors.InvalidUpgradeMagic(); + } + + function entropyUpgradableMagic() public pure returns (uint32) { + return 0x66697265; + } +} diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol index 30092bfb..a325a105 100644 --- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol @@ -4,13 +4,15 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; import "@pythnetwork/entropy-sdk-solidity/EntropyStructs.sol"; -import "../contracts/entropy/Entropy.sol"; +import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import "./utils/EntropyTestUtils.t.sol"; +import "../contracts/entropy/EntropyUpgradable.sol"; // TODO // - fuzz test? contract EntropyTest is Test, EntropyTestUtils { - Entropy public random; + ERC1967Proxy public proxy; + EntropyUpgradable public random; uint128 pythFeeInWei = 7; @@ -33,8 +35,18 @@ contract EntropyTest is Test, EntropyTestUtils { uint128 MAX_UINT128 = 2 ** 128 - 1; bytes32 ALL_ZEROS = bytes32(uint256(0)); + address public owner = address(8); + address public admin = address(9); + address public admin2 = address(10); + function setUp() public { - random = new Entropy(pythFeeInWei, provider1, false); + EntropyUpgradable _random = new EntropyUpgradable(); + // deploy proxy contract and point it to implementation + proxy = new ERC1967Proxy(address(_random), ""); + // wrap in ABI to support easier calls + random = EntropyUpgradable(address(proxy)); + + random.initialize(owner, admin, pythFeeInWei, provider1, false); bytes32[] memory hashChain1 = generateHashChain( provider1, diff --git a/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol b/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol new file mode 100644 index 00000000..ee682994 --- /dev/null +++ b/target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.0; + +import "./utils/EntropyTestUtils.t.sol"; +import "../contracts/entropy/EntropyUpgradable.sol"; +import "./utils/EntropyTestContracts/EntropyDifferentMagic.t.sol"; +import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol"; + +contract EntropyAuthorized is Test, EntropyTestUtils { + ERC1967Proxy public proxy; + EntropyUpgradable public random; + EntropyUpgradable public random2; + EntropyDifferentMagic public randomDifferentMagic; + + address public owner = address(1); + address public admin = address(2); + address public admin2 = address(3); + + // We don't need to register providers for these tests + // We are just checking for the default provider, which + // only required an address. + address public provider1 = address(4); + address public provider2 = address(5); + + uint128 pythFeeInWei = 7; + + function setUp() public { + EntropyUpgradable _random = new EntropyUpgradable(); + // deploy proxy contract and point it to implementation + proxy = new ERC1967Proxy(address(_random), ""); + // wrap in ABI to support easier calls + random = EntropyUpgradable(address(proxy)); + // to test for upgrade + random2 = new EntropyUpgradable(); + randomDifferentMagic = new EntropyDifferentMagic(); + + random.initialize(owner, admin, pythFeeInWei, provider1, false); + } + + function testSetAdminByAdmin() public { + vm.prank(admin); + random.setAdmin(admin2); + assertEq(random.getAdmin(), admin2); + } + + function testSetAdminByOwner() public { + vm.prank(owner); + random.setAdmin(admin2); + assertEq(random.getAdmin(), admin2); + } + + function testExpectRevertSetAdminByUnauthorized() public { + vm.expectRevert(EntropyErrors.Unauthorized.selector); + vm.prank(admin2); + random.setAdmin(admin); + } + + function testSetPythFeeByAdmin() public { + vm.prank(admin); + random.setPythFee(10); + assertEq(random.getPythFee(), 10); + } + + function testSetPythFeeByOwner() public { + vm.prank(owner); + random.setPythFee(10); + assertEq(random.getPythFee(), 10); + } + + function testExpectRevertSetPythFeeByUnauthorized() public { + vm.expectRevert(EntropyErrors.Unauthorized.selector); + vm.prank(admin2); + random.setPythFee(10); + } + + function testSetDefaultProviderByOwner() public { + vm.prank(owner); + random.setDefaultProvider(provider2); + assertEq(random.getDefaultProvider(), provider2); + } + + function testSetDefaultProviderByAdmin() public { + vm.prank(admin); + random.setDefaultProvider(provider2); + assertEq(random.getDefaultProvider(), provider2); + } + + function testExpectRevertSetDefaultProviderByUnauthorized() public { + vm.expectRevert(EntropyErrors.Unauthorized.selector); + vm.prank(admin2); + random.setDefaultProvider(provider2); + } + + function testUpgradeByOwner() public { + vm.prank(owner); + random.upgradeTo(address(random2)); + } + + function testExpectRevertUpgradeByAdmin() public { + // The message is returned by openzepplin upgrade contracts + vm.expectRevert("Ownable: caller is not the owner"); + vm.prank(admin); + random.upgradeTo(address(random2)); + } + + function testExpectRevertUpgradeByUnauthorized() public { + // The message is returned by openzepplin upgrade contracts + vm.expectRevert("Ownable: caller is not the owner"); + vm.prank(provider1); + random.upgradeTo(address(random2)); + } + + // There can be another case that the magic function doesn't + // exist but it's fine. (It will revert with no reason) + // The randomDifferentMagic contract do have a magic in this case + function testExpectRevertDifferentMagicContractUpgrade() public { + vm.expectRevert(EntropyErrors.InvalidUpgradeMagic.selector); + vm.prank(owner); + random.upgradeTo(address(randomDifferentMagic)); + } +} diff --git a/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol b/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol index 0aa3a366..86216b0a 100644 --- a/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol +++ b/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol @@ -4,14 +4,17 @@ pragma solidity ^0.8.0; import "forge-std/Test.sol"; import "@pythnetwork/entropy-sdk-solidity/EntropyStructs.sol"; -import "../contracts/entropy/Entropy.sol"; +import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; + import "./utils/EntropyTestUtils.t.sol"; +import "../contracts/entropy/EntropyUpgradable.sol"; // TODO // - what's the impact of # of in-flight requests on gas usage? More requests => more hashes to // verify the provider's value. contract EntropyGasBenchmark is Test, EntropyTestUtils { - Entropy public random; + ERC1967Proxy public proxy; + EntropyUpgradable public random; uint128 pythFeeInWei = 7; @@ -23,7 +26,19 @@ contract EntropyGasBenchmark is Test, EntropyTestUtils { address public user1 = address(3); function setUp() public { - random = new Entropy(pythFeeInWei, provider1, true); + EntropyUpgradable _random = new EntropyUpgradable(); + // deploy proxy contract and point it to implementation + proxy = new ERC1967Proxy(address(_random), ""); + // wrap in ABI to support easier calls + random = EntropyUpgradable(address(proxy)); + + random.initialize( + address(4), + address(5), + pythFeeInWei, + provider1, + false + ); bytes32[] memory hashChain1 = generateHashChain( provider1, diff --git a/target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol b/target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol new file mode 100644 index 00000000..66af7b27 --- /dev/null +++ b/target_chains/ethereum/contracts/forge-test/utils/EntropyTestContracts/EntropyDifferentMagic.t.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol"; + +contract EntropyDifferentMagic is + Initializable, + OwnableUpgradeable, + UUPSUpgradeable +{ + function initialize() public initializer { + __Ownable_init(); + __UUPSUpgradeable_init(); + } + + // /// Ensures the contract cannot be uninitialized and taken over. + // /// @custom:oz-upgrades-unsafe-allow constructor + constructor() initializer {} + + // // Only allow the owner to upgrade the proxy to a new implementation. + function _authorizeUpgrade(address) internal override onlyOwner {} + + function magicCheck() internal view { + // Calling a method using `this.` will cause a contract call that will use + // the new contract. This call will fail if the method does not exists or the magic + // is different. + if (this.entropyUpgradableMagic() != 0x666972) + revert EntropyErrors.InvalidUpgradeMagic(); + } + + function entropyUpgradableMagic() public pure returns (uint32) { + return 0x666972; + } +} diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol index dd8afe61..7b7fbe54 100644 --- a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol +++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol @@ -18,4 +18,8 @@ library EntropyErrors { error InsufficientFee(); // Either the user's or the provider's revealed random values did not match their commitment. error IncorrectRevelation(); + // Governance message is invalid (e.g., deserialization error). + error InvalidUpgradeMagic(); + // Unauthorized (e.g., invalid admin or owner authorisation). + error Unauthorized(); }