Merge pull request #133 from bancorprotocol/voucher-immutability

Voucher immutability

contracts/voucher/Voucher.sol

@@ -20,9 +20,6 @@ contract Voucher is IVoucher, Upgradeable, ERC721Upgradeable, Utils {
 
     error BatchNotSupported();
 
-    // the minter role is required to mint/burn
-    bytes32 private constant ROLE_MINTER = keccak256("ROLE_MINTER");
-
     // a flag used to toggle between a unique URI per token / one global URI for all tokens
     bool private _useGlobalURI;
 
@@ -35,8 +32,11 @@ contract Voucher is IVoucher, Upgradeable, ERC721Upgradeable, Utils {
     // a mapping between an owner to its tokenIds
     mapping(address => EnumerableSet.UintSet) internal _ownedTokens;
 
+    // controller address - used to mint / burn
+    address private _controller;
+
     // upgrade forward-compatibility storage gap
-    uint256[MAX_GAP - 4] private __gap;
+    uint256[MAX_GAP - 5] private __gap;
 
     /**
      @dev triggered when updating useGlobalURI
@@ -95,9 +95,6 @@ contract Voucher is IVoucher, Upgradeable, ERC721Upgradeable, Utils {
         string memory newBaseURI,
         string memory newBaseExtension
     ) internal onlyInitializing {
-        // set up administrative roles
-        _setRoleAdmin(ROLE_MINTER, ROLE_ADMIN);
-
         _useGlobalURI = newUseGlobalURI;
         __baseURI = newBaseURI;
         _baseExtension = newBaseExtension;
@@ -118,27 +115,27 @@ contract Voucher is IVoucher, Upgradeable, ERC721Upgradeable, Utils {
      * @inheritdoc Upgradeable
      */
     function version() public pure override(IVersioned, Upgradeable) returns (uint16) {
-        return 1;
+        return 2;
     }
 
     /**
-     * @dev returns the minter role
+     * @inheritdoc IVoucher
      */
-    function roleMinter() external pure returns (bytes32) {
-        return ROLE_MINTER;
+    function controller() external view returns (address) {
+        return _controller;
     }
 
     /**
      * @inheritdoc IVoucher
      */
-    function mint(address owner, uint256 tokenId) external onlyRoleMember(ROLE_MINTER) {
+    function mint(address owner, uint256 tokenId) external onlyController {
         _safeMint(owner, tokenId);
     }
 
     /**
      * @inheritdoc IVoucher
      */
-    function burn(uint256 tokenId) external onlyRoleMember(ROLE_MINTER) {
+    function burn(uint256 tokenId) external onlyController {
         _burn(tokenId);
     }
 
@@ -233,6 +230,32 @@ contract Voucher is IVoucher, Upgradeable, ERC721Upgradeable, Utils {
         emit UseGlobalURIUpdated(newUseGlobalURI);
     }
 
+    /**
+     * @dev sets the controller address
+     *
+     * requirements:
+     *
+     * - the caller must be the admin of this contract
+     * - controller address must not be set
+     */
+    function setController(address controllerAddress) external onlyAdmin {
+        if (_controller != address(0)) {
+            revert ControllerAlreadySet();
+        }
+        _controller = controllerAddress;
+    }
+
+    modifier onlyController() {
+        _onlyController();
+        _;
+    }
+
+    function _onlyController() private view {
+        if (msg.sender != _controller) {
+            revert OnlyController();
+        }
+    }
+
     /**
      * @dev Base URI for computing {tokenURI}. If set, the resulting URI for each
      * token will be the concatenation of the `baseURI`, `tokenId`

contracts/voucher/interfaces/IVoucher.sol

@@ -9,12 +9,20 @@ import { IUpgradeable } from "../../utility/interfaces/IUpgradeable.sol";
  * @dev Voucher interface
  */
 interface IVoucher is IUpgradeable, IERC721Upgradeable {
+    error ControllerAlreadySet();
+    error OnlyController();
+
+    /**
+     * @dev returns the controller address
+     */
+    function controller() external view returns (address);
+
     /**
      * @dev creates a new voucher token for the given strategyId, transfers it to the owner
      *
      * requirements:
      *
-     * - the caller must have the ROLE_MINTER privilege
+     * - the caller must be the controller address
      *
      */
     function mint(address owner, uint256 strategyId) external;
@@ -24,7 +32,7 @@ interface IVoucher is IUpgradeable, IERC721Upgradeable {
      *
      * requirements:
      *
-     * - the caller must have the ROLE_MINTER privilege
+     * - the caller must be the controller address
      *
      */
     function burn(uint256 strategyId) external;

deploy/scripts/arbitrum/0003-CarbonController.ts

@@ -2,12 +2,11 @@ import { ZERO_ADDRESS } from '../../../utils/Constants';
 import {
     DeployedContracts,
     deployProxy,
-    grantRole,
+    execute,
     InstanceName,
     setDeploymentMetadata,
     upgradeProxy
 } from '../../../utils/Deploy';
-import { Roles } from '../../../utils/Roles';
 import { DeployFunction } from 'hardhat-deploy/types';
 import { HardhatRuntimeEnvironment } from 'hardhat/types';
 
@@ -29,10 +28,11 @@ const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironm
         args: [voucher.address, carbonController.address]
     });
 
-    await grantRole({
+    // Set the carbon controller address in the voucher contract
+    await execute({
         name: InstanceName.Voucher,
-        id: Roles.Voucher.ROLE_MINTER,
-        member: carbonController.address,
+        methodName: 'setController',
+        args: [carbonController.address],
         from: deployer
     });
 

deploy/scripts/base/0003-CarbonController.ts

@@ -2,12 +2,11 @@ import { ZERO_ADDRESS } from '../../../utils/Constants';
 import {
     DeployedContracts,
     deployProxy,
-    grantRole,
+    execute,
     InstanceName,
     setDeploymentMetadata,
     upgradeProxy
 } from '../../../utils/Deploy';
-import { Roles } from '../../../utils/Roles';
 import { DeployFunction } from 'hardhat-deploy/types';
 import { HardhatRuntimeEnvironment } from 'hardhat/types';
 
@@ -29,10 +28,11 @@ const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironm
         args: [voucher.address, carbonController.address]
     });
 
-    await grantRole({
+    // Set the carbon controller address in the voucher contract
+    await execute({
         name: InstanceName.Voucher,
-        id: Roles.Voucher.ROLE_MINTER,
-        member: carbonController.address,
+        methodName: 'setController',
+        args: [carbonController.address],
         from: deployer
     });
 

deploy/scripts/mainnet/0011-Voucher-upgrade.ts

@@ -0,0 +1,30 @@
+import { DeployedContracts, InstanceName, execute, setDeploymentMetadata, upgradeProxy } from '../../../utils/Deploy';
+import { DeployFunction } from 'hardhat-deploy/types';
+import { HardhatRuntimeEnvironment } from 'hardhat/types';
+
+/**
+ * @dev voucher immutability upgrade - replace minter role with controller role
+ */
+const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironment) => {
+    const { deployer } = await getNamedAccounts();
+
+    await upgradeProxy({
+        name: InstanceName.Voucher,
+        from: deployer,
+        args: []
+    });
+
+    const controller = await DeployedContracts.CarbonController.deployed();
+
+    // Set the carbon controller address in the voucher contract
+    await execute({
+        name: InstanceName.Voucher,
+        methodName: 'setController',
+        args: [controller.address],
+        from: deployer
+    });
+
+    return true;
+};
+
+export default setDeploymentMetadata(__filename, func);

deploy/scripts/mantle/0003-CarbonController.ts

@@ -2,12 +2,11 @@ import { ZERO_ADDRESS } from '../../../utils/Constants';
 import {
     DeployedContracts,
     deployProxy,
-    grantRole,
+    execute,
     InstanceName,
     setDeploymentMetadata,
     upgradeProxy
 } from '../../../utils/Deploy';
-import { Roles } from '../../../utils/Roles';
 import { DeployFunction } from 'hardhat-deploy/types';
 import { HardhatRuntimeEnvironment } from 'hardhat/types';
 
@@ -29,10 +28,11 @@ const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironm
         args: [voucher.address, carbonController.address]
     });
 
-    await grantRole({
+    // Set the carbon controller address in the voucher contract
+    await execute({
         name: InstanceName.Voucher,
-        id: Roles.Voucher.ROLE_MINTER,
-        member: carbonController.address,
+        methodName: 'setController',
+        args: [carbonController.address],
         from: deployer
     });
 

deploy/tests/arbitrum/0002-voucher.ts

@@ -14,7 +14,7 @@ describeDeployment(__filename, () => {
 
     it('should deploy and configure the voucher contract', async () => {
         expect(await proxyAdmin.getProxyAdmin(voucher.address)).to.equal(proxyAdmin.address);
-        expect(await voucher.version()).to.equal(1);
+        expect(await voucher.version()).to.equal(2);
     });
 
     it('voucher implementation should be initialized', async () => {

deploy/tests/base/0002-voucher.ts

@@ -14,7 +14,7 @@ describeDeployment(__filename, () => {
 
     it('should deploy and configure the voucher contract', async () => {
         expect(await proxyAdmin.getProxyAdmin(voucher.address)).to.equal(proxyAdmin.address);
-        expect(await voucher.version()).to.equal(1);
+        expect(await voucher.version()).to.equal(2);
     });
 
     it('voucher implementation should be initialized', async () => {

deploy/tests/mainnet/0011-voucher-upgrade.ts

@@ -0,0 +1,39 @@
+import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';
+import { ProxyAdmin, Voucher } from '../../../components/Contracts';
+import { DeployedContracts, describeDeployment, getNamedSigners } from '../../../utils/Deploy';
+import { expect } from 'chai';
+import { ethers } from 'hardhat';
+
+/**
+ * @dev Voucher immutability upgrade - replace minter role with controller
+ */
+describeDeployment(__filename, () => {
+    let proxyAdmin: ProxyAdmin;
+    let voucher: Voucher;
+    let deployer: SignerWithAddress;
+
+    beforeEach(async () => {
+        ({ deployer } = await getNamedSigners());
+        proxyAdmin = await DeployedContracts.ProxyAdmin.deployed();
+        voucher = await DeployedContracts.Voucher.deployed();
+    });
+
+    it('should deploy and configure the voucher contract', async () => {
+        expect(await proxyAdmin.getProxyAdmin(voucher.address)).to.equal(proxyAdmin.address);
+        expect(await voucher.version()).to.equal(2);
+    });
+
+    it('voucher implementation should be initialized', async () => {
+        const implementationAddress = await proxyAdmin.getProxyImplementation(voucher.address);
+        const voucherImpl: Voucher = await ethers.getContractAt('Voucher', implementationAddress);
+        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+        const tx = await voucherImpl.initialize(true, '1', '1', { gasLimit: 6000000 });
+        await expect(tx.wait()).to.be.reverted;
+    });
+
+    it("admin shouldn't be able to change controller address", async () => {
+        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+        const tx = await voucher.connect(deployer).setController(deployer.address, { gasLimit: 6000000 });
+        await expect(tx.wait()).to.be.reverted;
+    });
+});

deploy/tests/mainnet/carbon.ts

@@ -537,7 +537,10 @@ import { ethers, getNamedAccounts } from 'hardhat';
                     it(`(${token0}->${token1})`, async () => {
                         const _token0 = tokens[token0] ? tokens[token0].address : ZERO_ADDRESS;
                         const _token1 = tokens[token1] ? tokens[token1].address : ZERO_ADDRESS;
-                        const tx = await carbonController.createStrategy(_token0, _token1, [order, order]);
+                        // hardcoding gas limit to avoid gas estimation attempts (which get rejected instead of reverted)
+                        const tx = await carbonController.createStrategy(_token0, _token1, [order, order], {
+                            gasLimit: 6000000
+                        });
                         await expect(tx.wait()).to.be.reverted;
                     });
                 }

deploy/tests/mantle/0002-voucher.ts

@@ -14,7 +14,7 @@ describeDeployment(__filename, () => {
 
     it('should deploy and configure the voucher contract', async () => {
         expect(await proxyAdmin.getProxyAdmin(voucher.address)).to.equal(proxyAdmin.address);
-        expect(await voucher.version()).to.equal(1);
+        expect(await voucher.version()).to.equal(2);
     });
 
     it('voucher implementation should be initialized', async () => {

test/forge/Strategies.t.sol

@@ -1671,7 +1671,7 @@ contract StrategiesTest is TestFixture {
         Order memory order = generateTestOrder();
         order.y = 0;
         // create strategy
-        vm.expectRevert(AccessDenied.selector);
+        vm.expectRevert(IVoucher.OnlyController.selector);
         newCarbonController.createStrategy(token0, token1, [order, order]);
 
         vm.stopPrank();

test/forge/TestFixture.t.sol

@@ -113,8 +113,8 @@ contract TestFixture is Test {
         // Set Carbon Controller address
         carbonController = TestCarbonController(payable(address(carbonController)));
 
-        // Grant minter role for voucher to carbon controller
-        voucher.grantRole(voucher.roleMinter(), address(carbonController));
+        // Set controller address on voucher
+        voucher.setController(address(carbonController));
 
         vm.stopPrank();
     }