SIP-127: deploy ERC-1167 proxies for virtual synths (Synthetixio#1191)

Author: sohkai

contracts/ExchangerWithVirtualSynth.sol

@@ -5,23 +5,44 @@ import "./Exchanger.sol";
 
 // Internal references
 import "./interfaces/IVirtualSynth.sol";
+import "./MinimalProxyFactory.sol";
 import "./VirtualSynth.sol";
 
 // https://docs.synthetix.io/contracts/source/contracts/exchangerwithvirtualsynth
-contract ExchangerWithVirtualSynth is Exchanger {
-    constructor(address _owner, address _resolver) public Exchanger(_owner, _resolver) {}
+contract ExchangerWithVirtualSynth is MinimalProxyFactory, Exchanger {
+    constructor(address _owner, address _resolver) public MinimalProxyFactory() Exchanger(_owner, _resolver) {}
+
+    /* ========== ADDRESS RESOLVER CONFIGURATION ========== */
+
+    bytes32 private constant CONTRACT_VIRTUALSYNTH_MASTERCOPY = "VirtualSynthMastercopy";
+
+    function resolverAddressesRequired() public view returns (bytes32[] memory addresses) {
+        bytes32[] memory existingAddresses = Exchanger.resolverAddressesRequired();
+        bytes32[] memory newAddresses = new bytes32[](1);
+        newAddresses[0] = CONTRACT_VIRTUALSYNTH_MASTERCOPY;
+        addresses = combineArrays(existingAddresses, newAddresses);
+    }
+
+    /* ========== INTERNAL FUNCTIONS ========== */
+
+    function _virtualSynthMastercopy() internal view returns (address) {
+        return requireAndGetAddress(CONTRACT_VIRTUALSYNTH_MASTERCOPY);
+    }
 
     function _createVirtualSynth(
         IERC20 synth,
         address recipient,
         uint amount,
         bytes32 currencyKey
-    ) internal returns (IVirtualSynth vSynth) {
+    ) internal returns (IVirtualSynth) {
         // prevent inverse synths from being allowed due to purgeability
         require(currencyKey[0] != 0x69, "Cannot virtualize this synth");
 
-        vSynth = new VirtualSynth(synth, resolver, recipient, amount, currencyKey);
+        VirtualSynth vSynth = VirtualSynth(_cloneAsMinimalProxy(_virtualSynthMastercopy(), "Could not create new vSynth"));
+        vSynth.initialize(synth, resolver, recipient, amount, currencyKey);
         emit VirtualSynthCreated(address(synth), recipient, address(vSynth), currencyKey, amount);
+
+        return IVirtualSynth(address(vSynth));
     }
 
     event VirtualSynthCreated(

contracts/MinimalProxyFactory.sol

@@ -0,0 +1,31 @@
+pragma solidity ^0.5.16;
+
+// https://docs.synthetix.io/contracts/source/contracts/minimalproxyfactory
+contract MinimalProxyFactory {
+    function _cloneAsMinimalProxy(address _base, string memory _revertMsg) internal returns (address clone) {
+        bytes memory createData = _generateMinimalProxyCreateData(_base);
+
+        assembly {
+            clone := create(
+                0, // no value
+                add(createData, 0x20), // data
+                55 // data is always 55 bytes (10 constructor + 45 code)
+            )
+        }
+
+        // If CREATE fails for some reason, address(0) is returned
+        require(clone != address(0), _revertMsg);
+    }
+
+    function _generateMinimalProxyCreateData(address _base) internal pure returns (bytes memory) {
+        return
+            abi.encodePacked(
+                //---- constructor -----
+                bytes10(0x3d602d80600a3d3981f3),
+                //---- proxy code -----
+                bytes10(0x363d3d373d3d3d363d73),
+                _base,
+                bytes15(0x5af43d82803e903d91602b57fd5bf3)
+            );
+    }
+}

contracts/VirtualSynth.sol

@@ -16,6 +16,10 @@ import "./interfaces/IExchanger.sol";
 import "openzeppelin-solidity-2.3.0/contracts/token/ERC20/IERC20.sol";
 
 // https://docs.synthetix.io/contracts/source/contracts/virtualsynth
+// Note: this contract should be treated as an abstract contract and should not be directly deployed.
+//       On higher versions of solidity, it would be marked with the `abstract` keyword.
+//       This contracts implements logic that is only intended to be accessed behind a proxy.
+//       For the deployed "mastercopy" version, see VirtualSynthMastercopy.
 contract VirtualSynth is ERC20, IVirtualSynth {
     using SafeMath for uint;
     using SafeDecimalMath for uint;
@@ -35,13 +39,18 @@ contract VirtualSynth is ERC20, IVirtualSynth {
 
     bytes32 public currencyKey;
 
-    constructor(
+    bool public initialized = false;
+
+    function initialize(
         IERC20 _synth,
         IAddressResolver _resolver,
         address _recipient,
         uint _amount,
         bytes32 _currencyKey
-    ) public ERC20() {
+    ) external {
+        require(!initialized, "vSynth already initialized");
+        initialized = true;
+
         synth = _synth;
         resolver = _resolver;
         currencyKey = _currencyKey;
@@ -51,6 +60,9 @@ contract VirtualSynth is ERC20, IVirtualSynth {
         _mint(_recipient, _amount);
 
         initialSupply = _amount;
+
+        // Note: the ERC20 base contract does not have a constructor, so we do not have to worry
+        // about initializing its state separately
     }
 
     // INTERNALS

contracts/VirtualSynthMastercopy.sol

@@ -0,0 +1,13 @@
+pragma solidity ^0.5.16;
+
+import "./VirtualSynth.sol";
+
+// https://docs.synthetix.io/contracts/source/contracts/virtualsynthmastercopy
+// Note: this is the "frozen" mastercopy of the VirtualSynth contract that should be linked to from
+//       proxies.
+contract VirtualSynthMastercopy is VirtualSynth {
+    constructor() public ERC20() {
+        // Freeze mastercopy on deployment so it can never be initialized with real arguments
+        initialized = true;
+    }
+}

contracts/test-helpers/MockPayable.sol

@@ -0,0 +1,10 @@
+pragma solidity ^0.5.16;
+
+contract MockPayable {
+    uint256 public paidTimes;
+
+    function pay() external payable {
+        require(msg.value > 0, "No value paid");
+        paidTimes = paidTimes + 1;
+    }
+}

contracts/test-helpers/MockReverter.sol

@@ -0,0 +1,7 @@
+pragma solidity ^0.5.16;
+
+contract MockReverter {
+    function revertWithMsg(string calldata _msg) external pure {
+        revert(_msg);
+    }
+}

contracts/test-helpers/TestableMinimalProxyFactory.sol

@@ -0,0 +1,19 @@
+pragma solidity ^0.5.16;
+
+import "../MinimalProxyFactory.sol";
+
+
+contract TestableMinimalProxyFactory is MinimalProxyFactory {
+    function cloneAsMinimalProxy(address _base, string calldata _revertMsg) external returns (address clone) {
+        clone = _cloneAsMinimalProxy(_base, _revertMsg);
+        emit CloneDeployed(clone, _base);
+
+        return clone;
+    }
+
+    function generateMinimalProxyCreateData(address _base) external pure returns (bytes memory) {
+        return _generateMinimalProxyCreateData(_base);
+    }
+
+    event CloneDeployed(address clone, address base);
+}

index.js

@@ -95,6 +95,7 @@ const constants = {
 	AST_FILENAME: 'asts.json',
 
 	ZERO_ADDRESS: '0x' + '0'.repeat(40),
+	ZERO_BYTES32: '0x' + '0'.repeat(64),
 
 	OVM_MAX_GAS_LIMIT: '8999999',
 

publish/deployed/local/config.json

@@ -62,6 +62,9 @@
 	"Exchanger": {
 		"deploy": true
 	},
+	"VirtualSynthMastercopy": {
+		"deploy": true
+	},
 	"ExchangeState": {
 		"deploy": true
 	},

publish/releases.json

@@ -175,5 +175,14 @@
 		},
 		"sources": [],
 		"sips": [113]
+	},
+	{
+		"name": "Alnilam",
+		"version": {
+			"major": 2,
+			"minor": 42
+		},
+		"sources": ["Exchanger", "VirtualSynthMastercopy"],
+		"sips": [127]
 	}
 ]

publish/src/commands/deploy.js

@@ -772,12 +772,26 @@ const deploy = async ({
 		args: [account, addressOf(readProxyForResolver)],
 	});
 
-	const exchanger = await deployer.deployContract({
-		name: 'Exchanger',
-		source: useOvm ? 'Exchanger' : 'ExchangerWithVirtualSynth',
-		deps: ['AddressResolver'],
-		args: [account, addressOf(readProxyForResolver)],
-	});
+	let exchanger;
+	if (useOvm) {
+		exchanger = await deployer.deployContract({
+			name: 'Exchanger',
+			source: 'Exchanger',
+			deps: ['AddressResolver'],
+			args: [account, addressOf(readProxyForResolver)],
+		});
+	} else {
+		exchanger = await deployer.deployContract({
+			name: 'Exchanger',
+			source: 'ExchangerWithVirtualSynth',
+			deps: ['AddressResolver'],
+			args: [account, addressOf(readProxyForResolver)],
+		});
+
+		await deployer.deployContract({
+			name: 'VirtualSynthMastercopy',
+		});
+	}
 
 	const exchangeState = await deployer.deployContract({
 		name: 'ExchangeState',

test/contracts/Exchanger.spec.js

@@ -3705,6 +3705,8 @@ contract('Exchanger (spec tests)', async accounts => {
 
 	describe('When using Synthetix', () => {
 		before(async () => {
+			const VirtualSynthMastercopy = artifacts.require('VirtualSynthMastercopy');
+
 			({
 				Exchanger: exchanger,
 				Synthetix: synthetix,
@@ -3742,6 +3744,10 @@ contract('Exchanger (spec tests)', async accounts => {
 					'FlexibleStorage',
 					'CollateralManager',
 				],
+				mocks: {
+					// Use a real VirtualSynthMastercopy so the spec tests can interrogate deployed vSynths
+					VirtualSynthMastercopy: await VirtualSynthMastercopy.new(),
+				},
 			}));
 
 			// Send a price update to guarantee we're not stale.

test/contracts/ExchangerWithVirtualSynth.behaviors.js

@@ -13,6 +13,8 @@ module.exports = function({ accounts }) {
 	});
 
 	beforeEach(async () => {
+		const VirtualSynthMastercopy = artifacts.require('VirtualSynthMastercopy');
+
 		({ mocks: this.mocks, resolver: this.resolver } = await prepareSmocks({
 			contracts: [
 				'DebtCache',
@@ -26,6 +28,10 @@ module.exports = function({ accounts }) {
 				'SystemStatus',
 				'TradingRewards',
 			],
+			mocks: {
+				// Use a real VirtualSynthMastercopy so the unit tests can interrogate deployed vSynths
+				VirtualSynthMastercopy: await VirtualSynthMastercopy.new(),
+			},
 			accounts: accounts.slice(10), // mock using accounts after the first few
 		}));
 	});
@@ -99,7 +105,7 @@ module.exports = function({ accounts }) {
 				cb();
 			});
 		},
-		whenMockedASynthToIssueAmdBurn: cb => {
+		whenMockedASynthToIssueAndBurn: cb => {
 			describe(`when mocked a synth to burn`, () => {
 				beforeEach(async () => {
 					// create and share the one synth for all Issuer.synths() calls

test/contracts/ExchangerWithVirtualSynth.unit.js

@@ -1,10 +1,15 @@
 'use strict';
 
-const { artifacts, contract } = require('hardhat');
+const { artifacts, contract, web3 } = require('hardhat');
 
 const { assert } = require('./common');
 
-const { onlyGivenAddressCanInvoke, ensureOnlyExpectedMutativeFunctions } = require('./helpers');
+const {
+	onlyGivenAddressCanInvoke,
+	ensureOnlyExpectedMutativeFunctions,
+	getEventByName,
+	buildMinimalProxyCode,
+} = require('./helpers');
 
 const { toBytes32 } = require('../..');
 
@@ -73,7 +78,7 @@ contract('ExchangerWithVirtualSynth (unit tests)', async accounts => {
 										() => {
 											behaviors.whenMockedEffectiveRateAsEqual(() => {
 												behaviors.whenMockedLastNRates(() => {
-													behaviors.whenMockedASynthToIssueAmdBurn(() => {
+													behaviors.whenMockedASynthToIssueAndBurn(() => {
 														behaviors.whenMockedExchangeStatePersistance(() => {
 															it('it reverts trying to create a virtual synth with no supply', async () => {
 																await assert.revert(
@@ -123,7 +128,7 @@ contract('ExchangerWithVirtualSynth (unit tests)', async accounts => {
 								() => {
 									behaviors.whenMockedEffectiveRateAsEqual(() => {
 										behaviors.whenMockedLastNRates(() => {
-											behaviors.whenMockedASynthToIssueAmdBurn(() => {
+											behaviors.whenMockedASynthToIssueAndBurn(() => {
 												behaviors.whenMockedExchangeStatePersistance(() => {
 													describe('when invoked', () => {
 														let txn;
@@ -147,13 +152,14 @@ contract('ExchangerWithVirtualSynth (unit tests)', async accounts => {
 																recipient: owner,
 															});
 														});
-														describe('when interrogating the Virtual Synths construction params', () => {
+														describe('when interrogating the Virtual Synths', () => {
 															let vSynth;
 															beforeEach(async () => {
-																const { vSynth: vSynthAddress } = txn.logs.find(
-																	({ event }) => event === 'VirtualSynthCreated'
-																).args;
-																vSynth = await artifacts.require('VirtualSynth').at(vSynthAddress);
+																const VirtualSynth = artifacts.require('VirtualSynth');
+																vSynth = await VirtualSynth.at(
+																	getEventByName({ tx: txn, name: 'VirtualSynthCreated' }).args
+																		.vSynth
+																);
 															});
 															it('the vSynth has the correct synth', async () => {
 																assert.equal(
@@ -175,6 +181,13 @@ contract('ExchangerWithVirtualSynth (unit tests)', async accounts => {
 																);
 																assert.equal(this.mocks.synth.smocked.issue.calls[0][1], amount);
 															});
+															it('the vSynth is an ERC-1167 minimal proxy instead of a full Virtual Synth', async () => {
+																const vSynthCode = await web3.eth.getCode(vSynth.address);
+																assert.equal(
+																	vSynthCode,
+																	buildMinimalProxyCode(this.mocks.VirtualSynthMastercopy.address)
+																);
+															});
 														});
 													});
 												});