Merge pull request #66 from renproject/shifter-transfer-token

Allow the Shifter to transfer the ownership of its token

contracts/Shifter/Shifter.sol

@@ -58,20 +58,26 @@ contract Shifter is Ownable {
     // Public functions ////////////////////////////////////////////////////////
 
     /// @notice Claims ownership of the token passed in to the constructor.
-    ///         `transferStoreOwnership` must have previously been called.
-    ///         Anyone can call this function.
+    /// `transferStoreOwnership` must have previously been called.
+    /// Anyone can call this function.
     function claimTokenOwnership() public {
         token.claimOwnership();
     }
 
-    /// @notice Allow the mint authority to update the fee recipient.
+    /// @notice Allow the owner to update the owner of the ERC20Shifted token.
+    function transferTokenOwnership(Shifter _nextTokenOwner) public onlyOwner {
+        token.transferOwnership(address(_nextTokenOwner));
+        _nextTokenOwner.claimTokenOwnership();
+    }
+
+    /// @notice Allow the owner to update the fee recipient.
     ///
     /// @param _nextMintAuthority The address to start paying fees to.
     function updateMintAuthority(address _nextMintAuthority) public onlyOwner {
         mintAuthority = _nextMintAuthority;
     }
 
-    /// @notice Allow the mint authority to update the fee recipient.
+    /// @notice Allow the owner to update the fee recipient.
     ///
     /// @param _nextFeeRecipient The address to start paying fees to.
     function updateFeeRecipient(address _nextFeeRecipient) public onlyOwner {
@@ -81,7 +87,7 @@ contract Shifter is Ownable {
         feeRecipient = _nextFeeRecipient;
     }
 
-    /// @notice Allow the mint authority to update the fee.
+    /// @notice Allow the owner to update the fee.
     ///
     /// @param _nextFee The new fee for minting and burning.
     function updateFee(uint16 _nextFee) public onlyOwner {

contracts/Shifter/ShifterRegistry.sol

@@ -44,7 +44,7 @@ contract ShifterRegistry is Claimable {
 
         // Add to list of shifters
         LinkedList.append(shifterList, _shifterAddress);
-        
+
         // Add to list of shifted tokens
         LinkedList.append(shiftedTokenList, _tokenAddress);
 
@@ -67,7 +67,7 @@ contract ShifterRegistry is Claimable {
 
         // Remove to list of shifters
         LinkedList.remove(shifterList, currentShifter);
-        
+
         // Add to list of shifted tokens
         LinkedList.append(shifterList, _newShifterAddress);
 
@@ -106,7 +106,7 @@ contract ShifterRegistry is Claimable {
         } else {
             count = _count;
         }
-        
+
         address[] memory shifters = new address[](count);
 
         // Begin with the first node in the list

test/CompatibleERC20.ts

@@ -1,7 +1,7 @@
 import BN from "bn.js";
 
-import "./helper/testUtils";
 import { CompatibleERC20TestInstance, ReturnsFalseTokenInstance } from "../types/truffle-contracts";
+import "./helper/testUtils";
 
 const CompatibleERC20Test = artifacts.require("CompatibleERC20Test");
 const NormalToken = artifacts.require("NormalToken");
@@ -32,7 +32,7 @@ contract("CompatibleERC20", (accounts) => {
             const FEE = VALUE.mul(new BN(testCase.fees)).div(new BN(1000));
 
             before(async () => {
-                token = await testCase.contract.new();
+                token = await testCase.contract.new() as ReturnsFalseTokenInstance;
             });
 
             it("approve and transferFrom", async () => {

test/DarknodeRegistry.ts

@@ -1,10 +1,12 @@
 import BN from "bn.js";
 
 import {
-    ID, MINIMUM_BOND, MINIMUM_EPOCH_INTERVAL, MINIMUM_POD_SIZE,
-    NULL, PUBK, waitForEpoch,
+    DarknodeRegistryInstance, DarknodeRegistryStoreInstance, DarknodeSlasherInstance,
+    RenTokenInstance,
+} from "../types/truffle-contracts";
+import {
+    ID, MINIMUM_BOND, MINIMUM_EPOCH_INTERVAL, MINIMUM_POD_SIZE, NULL, PUBK, waitForEpoch,
 } from "./helper/testUtils";
-import { RenTokenInstance, DarknodeRegistryStoreInstance, DarknodeRegistryInstance, DarknodeSlasherInstance } from "../types/truffle-contracts";
 
 const RenToken = artifacts.require("RenToken");
 const DarknodeRegistryStore = artifacts.require("DarknodeRegistryStore");

test/Shifter.ts

@@ -1,5 +1,6 @@
 import BN from "bn.js";
 import { ecrecover, ecsign, pubToAddress } from "ethereumjs-util";
+import { Account } from "web3-eth-accounts";
 import { keccak256 } from "web3-utils";
 
 import {
@@ -18,15 +19,15 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
 
     // We generate a new account so that we have access to its private key for
     // `ecsign`. Web3's sign functions all prefix the message being signed.
-    let mintAuthority;
-    let privKey;
+    let mintAuthority: Account;
+    let privKey: Buffer;
 
     const feeInBips = new BN(10);
 
     before(async () => {
         zbtc = await zBTC.new();
         mintAuthority = web3.eth.accounts.create();
-        privKey = Buffer.from(mintAuthority.privateKey.slice(2), "hex")
+        privKey = Buffer.from(mintAuthority.privateKey.slice(2), "hex");
 
         btcShifter = await BTCShifter.new(
             zbtc.address,
@@ -39,7 +40,8 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
         await btcShifter.claimTokenOwnership();
     });
 
-    const removeFee = (value: number | BN, bips: number | BN) => new BN(value).sub(new BN(value).mul(new BN(bips)).div(new BN(10000)));
+    const removeFee = (value: number | BN, bips: number | BN) =>
+        new BN(value).sub(new BN(value).mul(new BN(bips)).div(new BN(10000)));
 
     const mintTest = async (shifter: ShifterInstance, value: number | BN, shiftID = undefined) => {
         const nHash = randomBytes(32);
@@ -61,12 +63,15 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
         const _shiftID = await shifter.nextShiftID();
         (await shifter.shiftIn(pHash, value, nHash, sigString, { from: user }) as any)
             .should.emit.logs([
-                log("LogShiftIn", { _to: user, _amount: removeFee(value, 10), _shiftID: shiftID !== undefined ? shiftID : _shiftID }),
+                log(
+                    "LogShiftIn",
+                    { _to: user, _amount: removeFee(value, 10), _shiftID: shiftID !== undefined ? shiftID : _shiftID },
+                ),
             ]);
         (await zbtc.balanceOf(user)).should.bignumber.equal(balanceBefore.add(removeFee(value, 10)));
 
         return [pHash, nHash];
-    }
+    };
 
     const burnTest = async (shifter: ShifterInstance, value: number | BN, btcAddress?: string, shiftID = undefined) => {
         // Note: we don't use `||` because we want to pass in `""`
@@ -76,10 +81,18 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
         const _shiftID = await shifter.nextShiftID();
         (await shifter.shiftOut(btcAddress, value, { from: user }) as any)
             .should.emit.logs([
-                log("LogShiftOut", { _to: btcAddress, _amount: removeFee(value, 10), _shiftID: shiftID !== undefined ? shiftID : _shiftID, _indexedTo: keccak256(btcAddress) }),
+                log(
+                    "LogShiftOut",
+                    {
+                        _to: btcAddress,
+                        _amount: removeFee(value, 10),
+                        _shiftID: shiftID !== undefined ? shiftID : _shiftID,
+                        _indexedTo: keccak256(btcAddress),
+                    },
+                ),
             ]);
         (await zbtc.balanceOf(user)).should.bignumber.equal(balanceBefore.sub(new BN(value)));
-    }
+    };
 
     describe("can mint and burn", () => {
         const value = new BN(20000);
@@ -144,12 +157,13 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
 
             const hash = await btcShifter.hashForSignature(pHash, value.toNumber(), user, nHash);
 
-            let sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);
+            const sig = ecsign(Buffer.from(hash.slice(2), "hex"), privKey);
 
             // Invalid signature
             const altSig = {
                 ...sig,
-                s: new BN("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", "hex").sub(new BN(sig.s)).toArrayLike(Buffer, "be", 32),
+                s: new BN("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", "hex")
+                    .sub(new BN(sig.s)).toArrayLike(Buffer, "be", 32),
                 v: sig.v === 27 ? 28 : 27,
             };
             const altSigString = Ox(`${altSig.r.toString("hex")}${altSig.s.toString("hex")}${(altSig.v).toString(16)}`);
@@ -194,6 +208,57 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
         });
     });
 
+    describe("upgrading shifter", () => {
+        let newShifter: ShifterInstance;
+
+        it("can upgrade the shifter", async () => {
+            newShifter = await BTCShifter.new(
+                zbtc.address,
+                feeRecipient,
+                mintAuthority.address,
+                feeInBips,
+            );
+
+            // Fund and unlock the mintAuthority - not used currently but
+            // may be needed in the future.
+            /* await web3.eth.sendTransaction({ to: mintAuthority.address, from: owner, value: web3.utils.toWei("1") });
+             * await web3.eth.personal.importRawKey(mintAuthority.privateKey, "");
+             * await web3.eth.personal.unlockAccount(mintAuthority.address, "", 6000);
+             */
+
+            await (btcShifter.transferTokenOwnership(newShifter.address, { from: malicious }))
+                .should.be.rejectedWith(/caller is not the owner/);
+
+            await btcShifter.transferTokenOwnership(newShifter.address, { from: owner });
+            (await zbtc.owner()).should.equal(newShifter.address);
+        });
+
+        it("can mint and burn using new shifter", async () => {
+            const value = new BN(200000);
+            await mintTest(newShifter, value);
+            await burnTest(newShifter, removeFee(value, 10));
+        });
+
+        it("can't upgrade to an invalid shifter", async () => {
+            await (newShifter.transferTokenOwnership(malicious, { from: owner }))
+                .should.be.rejectedWith(/revert/);
+
+            await zbtc.claimOwnership({ from: malicious })
+                .should.be.rejectedWith(/caller is not the pending owner/);
+        });
+
+        it("can reset the upgrade", async () => {
+            // Trying to reset upgrade in btcShifter without owning the token
+            await (btcShifter.transferTokenOwnership(NULL, { from: owner }))
+                .should.be.rejectedWith(/caller is not the owner/);
+
+            // Upgrade newShifter to point to btcShifter
+            await newShifter.transferTokenOwnership(btcShifter.address, { from: owner });
+
+            (await zbtc.owner()).should.equal(btcShifter.address);
+        });
+    });
+
     describe("shifter registry", () => {
         let registry: ShifterRegistryInstance;
 
@@ -211,7 +276,7 @@ contract("Shifter", ([owner, feeRecipient, user, malicious]) => {
 
         it("can retrieve shifters", async () => {
             { // Try to register token with an existing symbol
-                const altZbtc = await zBTC.new()
+                const altZbtc = await zBTC.new();
                 await registry.setShifter(altZbtc.address, NULL)
                     .should.be.rejectedWith(/symbol already registered/);
             }

test/String.ts

@@ -5,28 +5,28 @@ const StringTest = artifacts.require("StringTest");
 
 contract("String", (accounts) => {
 
-    let String: StringTestInstance;
+    let StringInstance: StringTestInstance;
 
     before(async () => {
-        String = await StringTest.new();
+        StringInstance = await StringTest.new();
     });
 
     // Skipped for now due to an issue with the coverage tool.
     // The tests pass when run without coverage.
     it.skip("can add strings", async () => {
-        (await String.add4("1", "2", "3", "4"))
+        (await StringInstance.add4("1", "2", "3", "4"))
             .should.equal("1234");
     });
 
     it.skip("can convert addresses to hex strings", async () => {
-        (await String.fromAddress(accounts[0]))
+        (await StringInstance.fromAddress(accounts[0]))
             .should.equal(accounts[0].toLowerCase());
     });
 
     it.skip("can convert bytes32 to hex strings", async () => {
         const bytes32 = randomBytes(32);
 
-        (await String.fromBytes32(bytes32))
+        (await StringInstance.fromBytes32(bytes32))
             .should.equal(bytes32.toLowerCase());
     });
 });

test/Vesting.ts

@@ -1,10 +1,12 @@
-import BN from "bn.js";
-import { ecsign, keccak256 } from "ethereumjs-util";
 import BigNumber from "bignumber.js";
+import BN from "bn.js";
 import { rawEncode } from "ethereumjs-abi";
+import { ecsign, keccak256 } from "ethereumjs-util";
 
-import { BTCShifterInstance, VestingInstance, zBTCInstance, ShifterRegistryInstance } from "../types/truffle-contracts";
-import { increaseTime, NULL, Ox, randomBytes } from "./helper/testUtils";
+import {
+    BTCShifterInstance, ShifterRegistryInstance, VestingInstance, zBTCInstance,
+} from "../types/truffle-contracts";
+import { increaseTime, Ox, randomBytes } from "./helper/testUtils";
 
 const BTCShifter = artifacts.require("BTCShifter");
 const ShifterRegistry = artifacts.require("ShifterRegistry");
@@ -34,7 +36,7 @@ contract("Vesting", (accounts) => {
             mintAuthority.address,
             feeInBips,
         );
-            
+
         await zbtc.transferOwnership(btcShifter.address);
         await btcShifter.claimTokenOwnership();
 
@@ -57,15 +59,20 @@ contract("Vesting", (accounts) => {
             const startTime = 0;
             const pHash = keccak256(rawEncode(
                 ["address", "uint256", "uint16"],
-                [beneficiary, startTime, duration]
+                [beneficiary, startTime, duration],
             )).toString("hex");
 
-            const hashForSignature = await btcShifter.hashForSignature(Ox(pHash), amount.toNumber(), vesting.address, nonce);
+            const hashForSignature = await btcShifter.hashForSignature(
+                Ox(pHash),
+                amount.toNumber(),
+                vesting.address,
+                nonce,
+            );
             const sig = ecsign(Buffer.from(hashForSignature.slice(2), "hex"), privKey);
             const sigString = Ox(`${sig.r.toString("hex")}${sig.s.toString("hex")}${(sig.v).toString(16)}`);
 
             // User should have no schedules prior to adding.
-            let schedule = await vesting.schedules(beneficiary);
+            const schedule = await vesting.schedules(beneficiary);
             (schedule as any).startTime.should.bignumber.equal(new BN(0));
 
             await vesting.addVestingSchedule(
@@ -74,7 +81,7 @@ contract("Vesting", (accounts) => {
                 // Required
                 amount, nonce, sigString,
             );
-        }
+        };
 
         it("can add a vesting schedule", async () => {
             await addVestingSchedule();
@@ -90,7 +97,7 @@ contract("Vesting", (accounts) => {
             const amountBN = new BigNumber(amountAfterFee.toString());
             const resultBN = amountBN.times(new BigNumber(elapsedMonths)).dividedToIntegerBy(new BigNumber(duration));
             return new BN(resultBN.toString());
-        }
+        };
 
         it("can check claimable amount", async () => {
             let claimable = await vesting.calculateClaimable(beneficiary);
@@ -149,4 +156,4 @@ contract("Vesting", (accounts) => {
             claimable[1].should.bignumber.equal(amountAfterFee);
         });
     });
-});
+});