[STABLE-5628] Modify FiatTokenCeloV2_2 to support custom fee caller (c…

…irclefin#192)

## Summary
This pull request modifies `FiatTokenCeloV2_2` to allow greater
flexibility in who the fee caller for `debitGasFees` and `creditGasFees`
can be. Circle has decided to pursue an approach (pending Security, Rui
Maximo) in which yet another contract in the call stack calls
`debitGasFees` and `creditGasFees`. To that end, we cannot rely on our
assumption of 0x0 being the fee caller anymore.

## Detail
- Modified FTCV2.2 to account for a dynamic fee caller. This is stored
in a Keccak256 storage slot to prevent modification to the USDC storage
layout.
- Ensured that only the token owner can call `updateFeeCaller`, similar
to existing procedures for the blacklister, pauser, etc.
- Modified contracts where appropriate and added tests.

## Testing
See the tests.

## Documentation
- [`FiatTokenFeeAdapter` rollout
plan/timeline](https://docs.google.com/document/d/19qISUWAft_yCzg5lPrf41f2m3Bxgi7RQTIqeLZ6_Ghw/edit)
- [Product
document](https://docs.google.com/document/d/1YKVpFo9aPuBREX7eXcXinFLpskF5q9nUxvbBd0ifYl8)

## Sanitization

- [x] Will this commit be publicized? Depending on whether we squash
during open-sourcing, it may or may not be. @zhenghui-w is handling the
open-sourcing effort.

For PR author, have you:

- [x] Checked that PR title does not contain internal references? (eg.
internal code names, employee names)
- [x] Checked that commit messages do not contain internal references?
(eg. internal code names, employee names)
- [x] Checked that all new code contain the Apache 2.0 copyright license
header
- [x] Checked that all new third party dependencies use permissive
licenses

For reviewers, ensure that:

- The PR is squash merged
- JIRA story IDs are removed from the squash merge commit title

Refer to the [Open Source
Standard](https://drive.google.com/file/d/1OV5E9ZOBmyZeN877vPP02C92Mjm-qt4Z/view)
for more information about sanitization.

## Story
STABLE-5628

@types/AnyFiatTokenV2Instance.d.ts

@@ -20,16 +20,25 @@ import {
   FiatTokenV2Instance,
   FiatTokenV21Instance,
   FiatTokenV22Instance,
+  FiatTokenCeloV22Instance,
 } from "./generated";
 
-export interface FiatTokenV22InstanceExtended extends FiatTokenV22Instance {
+export interface OverloadedAAFunctions {
   permit?: typeof FiatTokenV2Instance.permit;
   transferWithAuthorization?: typeof FiatTokenV2Instance.transferWithAuthorization;
   receiveWithAuthorization?: typeof FiatTokenV2Instance.receiveWithAuthorization;
   cancelAuthorization?: typeof FiatTokenV2Instance.cancelAuthorization;
 }
 
+export interface FiatTokenV22InstanceExtended
+  extends FiatTokenV22Instance,
+    OverloadedAAFunctions {}
+export interface FiatTokenCeloV22InstanceExtended
+  extends FiatTokenCeloV22Instance,
+    OverloadedAAFunctions {}
+
 export type AnyFiatTokenV2Instance =
   | FiatTokenV2Instance
   | FiatTokenV21Instance
-  | FiatTokenV22InstanceExtended;
+  | FiatTokenV22InstanceExtended
+  | FiatTokenCeloV22InstanceExtended;

contracts/test/celo/MockFiatTokenCeloWithCustomVmAddress.sol → contracts/test/celo/MockFiatTokenCeloWithExposedFunctions.sol

@@ -24,25 +24,11 @@ import { FiatTokenCeloV2_2 } from "../../v2/celo/FiatTokenCeloV2_2.sol";
 
 /**
  * @dev This contract is the same as FiatTokenCeloV2_2, except, for testing,
- * it allows us to arbitrarily override the address that calls the debit
- * and credit functions. Truffle does not support impersonation of 0x0, so
- * we must call by using a different address.
+ * it allows us to call internal sensitive functions for testing. These
+ * external test functions are prefixed with "internal_" to differentiate
+ * them from the main internal functions.
  */
-contract MockFiatTokenCeloWithCustomVmAddress is FiatTokenCeloV2_2 {
-    address private _vmCallerAddress;
-
-    modifier onlyCeloVm() override {
-        require(
-            msg.sender == _vmCallerAddress,
-            "FiatTokenCeloV2_2: Only Celo VM can call"
-        );
-        _;
-    }
-
-    function setVmCallerAddress(address newVmCallerAddress) external {
-        _vmCallerAddress = newVmCallerAddress;
-    }
-
+contract MockFiatTokenCeloWithExposedFunctions is FiatTokenCeloV2_2 {
     function internal_debitedValue() external view returns (uint256) {
         return _debitedValue();
     }
@@ -51,7 +37,7 @@ contract MockFiatTokenCeloWithCustomVmAddress is FiatTokenCeloV2_2 {
         address from,
         address to,
         uint256 value
-    ) external onlyCeloVm {
+    ) external onlyFeeCaller {
         _transferReservedGas(from, to, value);
     }
 

contracts/v2/celo/FiatTokenCeloV2_2.sol

@@ -24,15 +24,53 @@ import { ICeloGasToken } from "../../interface/celo/ICeloGasToken.sol";
 
 contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
     using SafeMath for uint256;
+    event FeeCallerChanged(address indexed newAddress);
 
-    modifier onlyCeloVm() virtual {
+    /**
+     * @notice Constant containing the storage slot indicating the current fee caller of
+     * `debitGasFees` and `creditGasFees`. Only the fee caller should be able to represent
+     * this FiatToken during a gas lifecycle. This slot starts off indicating address(0)
+     * as the allowed fee caller, as the storage slot is empty.
+     * @dev This constant is the Keccak-256 hash of "com.circle.fiattoken.celo.feecaller" and is
+     * validated in the contract constructor. It does not occupy any storage slots, since
+     * constants are embedded in the bytecode of a smart contract. This is intentionally done
+     * so that the Celo variant of FiatToken can accommodate new state variables that may be
+     * added in future FiatToken versions.
+     */
+    bytes32
+        private constant FEE_CALLER_SLOT = 0xdca914aef3e4e19727959ebb1e70b58822e2c7b796d303902adc19513fcb4af5;
+
+    /**
+     * @notice Returns the current fee caller address allowed on `debitGasFees` and `creditGasFees`.
+     * @dev Though Solidity generates implicit viewers/getters on contract state, because we
+     * store the fee caller in a custom Keccak256 slot instead of a standard declaration, we
+     * need an explicit getter for that slot.
+     */
+    function feeCaller() public view returns (address value) {
+        assembly {
+            value := sload(FEE_CALLER_SLOT)
+        }
+    }
+
+    modifier onlyFeeCaller() virtual {
         require(
-            msg.sender == address(0),
-            "FiatTokenCeloV2_2: Only Celo VM can call"
+            msg.sender == feeCaller(),
+            "FiatTokenCeloV2_2: caller is not the fee caller"
         );
         _;
     }
 
+    /**
+     * @notice Updates the fee caller address.
+     * @param _newFeeCaller The address of the new pauser.
+     */
+    function updateFeeCaller(address _newFeeCaller) external onlyOwner {
+        assembly {
+            sstore(FEE_CALLER_SLOT, _newFeeCaller)
+        }
+        emit FeeCallerChanged(_newFeeCaller);
+    }
+
     /**
      * @notice Constant containing the storage slot indicating the debited value currently
      * reserved for a transaction's gas paid in this token. This value also serves as a flag
@@ -47,22 +85,25 @@ contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
     bytes32
         private constant DEBITED_VALUE_SLOT = 0xd90dccaa76fe7208f2f477143b6adabfeb5d4a5136982894dfc51177fa8eda28;
 
-    constructor() public {
-        assert(
-            DEBITED_VALUE_SLOT == keccak256("com.circle.fiattoken.celo.debit")
-        );
-    }
-
     function _debitedValue() internal view returns (uint256 value) {
         assembly {
             value := sload(DEBITED_VALUE_SLOT)
         }
     }
 
+    constructor() public {
+        assert(
+            DEBITED_VALUE_SLOT == keccak256("com.circle.fiattoken.celo.debit")
+        );
+        assert(
+            FEE_CALLER_SLOT == keccak256("com.circle.fiattoken.celo.feecaller")
+        );
+    }
+
     function debitGasFees(address from, uint256 value)
         external
         override
-        onlyCeloVm
+        onlyFeeCaller
         whenNotPaused
         notBlacklisted(from)
     {
@@ -81,17 +122,18 @@ contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
     function creditGasFees(
         address from,
         address feeRecipient,
+        // solhint-disable-next-line no-unused-vars
         address gatewayFeeRecipient,
         address communityFund,
         uint256 refund,
         uint256 tipTxFee,
-        // solhint-disable-next-line
+        // solhint-disable-next-line no-unused-vars
         uint256 gatewayFee,
         uint256 baseTxFee
     )
         external
         override
-        onlyCeloVm
+        onlyFeeCaller
         whenNotPaused
         notBlacklisted(from)
         notBlacklisted(feeRecipient)

test/helpers/index.ts

@@ -20,6 +20,7 @@ import { ecsign } from "ethereumjs-util";
 import { assert } from "chai";
 import { solidityPack } from "ethereumjs-abi";
 import {
+  FiatTokenCeloV22Instance,
   FiatTokenProxyInstance,
   FiatTokenV11Instance,
   FiatTokenV1Instance,
@@ -140,7 +141,8 @@ export async function initializeToVersion(
     | FiatTokenV11Instance
     | FiatTokenV2Instance
     | FiatTokenV21Instance
-    | FiatTokenV22Instance,
+    | FiatTokenV22Instance
+    | FiatTokenCeloV22Instance,
   version: "1" | "1.1" | "2" | "2.1" | "2.2",
   fiatTokenOwner: string,
   lostAndFound: string,

test/v2/FiatTokenV2_2.test.ts

@@ -20,6 +20,7 @@ import BN from "bn.js";
 import crypto from "crypto";
 import {
   AnyFiatTokenV2Instance,
+  FiatTokenCeloV22InstanceExtended,
   FiatTokenV22InstanceExtended,
 } from "../../@types/AnyFiatTokenV2Instance";
 import {
@@ -411,7 +412,7 @@ export function behavesLikeFiatTokenV22(
  * here we re-assign the overloaded method definition to the method name shorthand.
  */
 export function initializeOverloadedMethods(
-  fiatToken: FiatTokenV22InstanceExtended,
+  fiatToken: FiatTokenV22InstanceExtended | FiatTokenCeloV22InstanceExtended,
   signatureBytesType: SignatureBytesType
 ): void {
   if (signatureBytesType == SignatureBytesType.Unpacked) {

test/v2/celo/FiatTokenCeloV2_2.test.ts

@@ -16,7 +16,6 @@
  * limitations under the License.
  */
 
-import { AnyCeloFiatToken } from "../../../@types/AnyFiatTokenCeloInstance";
 import { usesOriginalStorageSlotPositions } from "../../helpers/storageSlots.behavior";
 
 import { expectRevert, initializeToVersion } from "../../helpers";
@@ -26,7 +25,11 @@ import {
   initializeOverloadedMethods,
 } from "../FiatTokenV2_2.test";
 import { SignatureBytesType } from "../GasAbstraction/helpers";
-import { AnyFiatTokenV2Instance } from "../../../@types/AnyFiatTokenV2Instance";
+import {
+  AnyFiatTokenV2Instance,
+  FiatTokenCeloV22InstanceExtended,
+} from "../../../@types/AnyFiatTokenV2Instance";
+import { FeeCallerChanged } from "../../../@types/generated/FiatTokenCeloV22";
 
 const SignatureChecker = artifacts.require("SignatureChecker");
 const FiatTokenCeloV2_2 = artifacts.require("FiatTokenCeloV2_2");
@@ -38,8 +41,9 @@ contract("FiatTokenCeloV2_2", (accounts) => {
   const feeRecipient = accounts[3];
   const gatewayFeeRecipient = accounts[4];
   const communityFund = accounts[5];
+  const feeCaller = fiatTokenOwner;
 
-  let fiatTokenCelo: AnyCeloFiatToken;
+  let fiatTokenCelo: FiatTokenCeloV22InstanceExtended;
 
   const getFiatToken = (
     signatureBytesType: SignatureBytesType
@@ -60,6 +64,8 @@ contract("FiatTokenCeloV2_2", (accounts) => {
       fiatTokenOwner,
       lostAndFound
     );
+    // Ensure we set the debit/credit fee caller for testing.
+    await fiatTokenCelo.updateFeeCaller(feeCaller, { from: fiatTokenOwner });
   });
 
   describe("initialized FiatTokenCeloV2_2 contract", () => {
@@ -84,14 +90,17 @@ contract("FiatTokenCeloV2_2", (accounts) => {
     });
   });
 
-  describe("onlyCeloVm", () => {
-    const errorMessage = "FiatTokenCeloV2_2: Only Celo VM can call";
+  describe("onlyFeeCaller", () => {
+    const errorMessage = "FiatTokenCeloV2_2: caller is not the fee caller";
 
-    it("should fail to call debitGasFees when caller is not 0x0", async () => {
-      await expectRevert(fiatTokenCelo.debitGasFees(from, 1), errorMessage);
+    it("should fail to call debitGasFees when caller is not fee caller", async () => {
+      await expectRevert(
+        fiatTokenCelo.debitGasFees(from, 1, { from: from }),
+        errorMessage
+      );
     });
 
-    it("should fail to call creditGasFees when caller is not 0x0", async () => {
+    it("should fail to call creditGasFees when caller is not fee caller", async () => {
       await expectRevert(
         fiatTokenCelo.creditGasFees(
           from,
@@ -101,10 +110,51 @@ contract("FiatTokenCeloV2_2", (accounts) => {
           1,
           1,
           1,
-          1
+          1,
+          { from: from }
         ),
         errorMessage
       );
     });
   });
+
+  describe("feeCaller", () => {
+    // See FiatTokenCeloV2_2#FEE_CALLER_SLOT.
+    const feeCallerSlot =
+      "0xdca914aef3e4e19727959ebb1e70b58822e2c7b796d303902adc19513fcb4af5";
+
+    it("should return correct feeCaller", async () => {
+      expect(await fiatTokenCelo.feeCaller())
+        .to.equal(feeCaller)
+        .to.equal(
+          web3.utils.toChecksumAddress(
+            await web3.eth.getStorageAt(fiatTokenCelo.address, feeCallerSlot)
+          )
+        );
+    });
+  });
+
+  describe("updateFeeCaller", () => {
+    it("should fail to update fee caller when sender is not token owner", async () => {
+      const ownableError = "Ownable: caller is not the owner";
+      await expectRevert(
+        fiatTokenCelo.updatePauser(from, { from: from }),
+        ownableError
+      );
+    });
+
+    it("should emit FeeCallerChanged event", async () => {
+      const newFeeCaller = communityFund;
+      const updateFeeCallerEvent = await fiatTokenCelo.updateFeeCaller(
+        newFeeCaller,
+        { from: fiatTokenOwner }
+      );
+
+      const log = updateFeeCallerEvent.logs[0] as Truffle.TransactionLog<
+        FeeCallerChanged
+      >;
+      assert.strictEqual(log.event, "FeeCallerChanged");
+      assert.strictEqual(log.args.newAddress, newFeeCaller);
+    });
+  });
 });