[STABLE-5167] Address Celo token audit and remove gatewayFee usage (c…

…irclefin#138)
zhenghui-w · Dec 11, 2023 · d28e0fd · d28e0fd
1 parent 1ed6f24
commit d28e0fd
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 40 deletions.
diff --git a/contracts/interface/celo/ICeloGasToken.sol b/contracts/interface/celo/ICeloGasToken.sol
@@ -49,7 +49,9 @@ interface ICeloGasToken {
      * currency. After the transaction is executed, unused gas is refunded to the sender and credited
      * to the various fee recipients via a call to `creditGasFees`. The events emitted by `creditGasFees`
      * reflect the *net* gas fee payments for the transaction. As an invariant, the original debited amount
-     * will always equal (refund + tipTxFee + gatewayFee + baseTxFee).
+     * will always equal (refund + tipTxFee + gatewayFee + baseTxFee). Though the amount debited in debitGasFees
+     * is always equal to (refund + tipTxFee + gatewayFee + baseTxFee), in practice, the gateway fee is never
+     * used (0) and should ideally be ignored except in the function signature to optimize gas savings.
      */
     function creditGasFees(
         address from,

diff --git a/contracts/v2/celo/FiatTokenCeloV2_2.sol b/contracts/v2/celo/FiatTokenCeloV2_2.sol
@@ -85,6 +85,7 @@ contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
         address communityFund,
         uint256 refund,
         uint256 tipTxFee,
+        // solhint-disable-next-line
         uint256 gatewayFee,
         uint256 baseTxFee
     )
@@ -94,12 +95,9 @@ contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
         whenNotPaused
         notBlacklisted(from)
         notBlacklisted(feeRecipient)
-        notBlacklisted(gatewayFeeRecipient)
         notBlacklisted(communityFund)
     {
-        uint256 creditValue = refund.add(tipTxFee).add(gatewayFee).add(
-            baseTxFee
-        );
+        uint256 creditValue = refund.add(tipTxFee).add(baseTxFee);
 
         // Because the Celo VM follows 1) debit, 2) main execution, and
         // 3) credit atomically as part of a single on-chain transaction,
@@ -111,13 +109,11 @@ contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
         );
 
         // The credit portion of the gas lifecycle can be summarized
-        // by the four Transfer events emitted here:
-        // token to debitee,
+        // by the three Transfer events emitted here:
+        // 0x0 to debitee,
         _transferReservedGas(address(0), from, creditValue);
         // debitee to validator,
         _transfer(from, feeRecipient, tipTxFee);
-        // debitee to gateway (deprecated),
-        _transfer(from, gatewayFeeRecipient, gatewayFee);
         // and debitee to Celo community fund.
         _transfer(from, communityFund, baseTxFee);
 
@@ -127,11 +123,18 @@ contract FiatTokenCeloV2_2 is FiatTokenV2_2, ICeloGasToken {
         }
     }
 
+    /**
+     * @dev This function differs from the standard _transfer function in that
+     * it does *not* check against the from and the to addresses being 0x0.
+     * This is needed for the usage of 0x0 as the gas intermediary.
+     * Further, this function validates that _value is > 0. For a comparison,
+     * see the FiatTokenV1#_transfer function.
+     */
     function _transferReservedGas(
         address _from,
         address _to,
         uint256 _value
-    ) internal onlyCeloVm {
+    ) internal {
         require(_value > 0, "FiatTokenCeloV2_2: Must reserve > 0 gas");
         require(
             _value <= _balanceOf(_from),

diff --git a/test/v2/celo/MockFiatTokenCeloWithCustomVmAddress.test.ts b/test/v2/celo/MockFiatTokenCeloWithCustomVmAddress.test.ts
@@ -50,7 +50,6 @@ contract("MockFiatTokenCeloWithCustomVmAddress", (accounts) => {
 
   const debitAmount = 1e6;
 
-  const onlyVmError = "FiatTokenCeloV2_2: Only Celo VM can call";
   const pausedError = "Pausable: paused";
   const blacklistedError = "Blacklistable: account is blacklisted";
   const debitInvariantError =
@@ -134,7 +133,8 @@ contract("MockFiatTokenCeloWithCustomVmAddress", (accounts) => {
 
   describe("creditGasFees", () => {
     const tipTxFee = 1e5;
-    const gatewayFee = 2e5;
+    // Invariant: the network does not actually make use of the gateway fee.
+    const gatewayFee = 0;
     const baseTxFee = 3e5;
 
     // The original debit always equals refund + tipTxFee + gatewayFee + baseTxFee.
@@ -318,25 +318,6 @@ contract("MockFiatTokenCeloWithCustomVmAddress", (accounts) => {
       );
     });
 
-    it("should fail when `gatewayFeeRecipient` is blacklisted", async () => {
-      await fiatToken.debitGasFees(from, debitAmount, { from: vmAddress });
-      await fiatToken.blacklist(gatewayFeeRecipient, { from: blacklister });
-      await expectRevert(
-        fiatToken.creditGasFees(
-          from,
-          feeRecipient,
-          gatewayFeeRecipient,
-          communityFund,
-          refund,
-          tipTxFee,
-          gatewayFee,
-          baseTxFee,
-          { from: vmAddress }
-        ),
-        blacklistedError
-      );
-    });
-
     it("should fail when `communityFund` is blacklisted", async () => {
       await fiatToken.debitGasFees(from, debitAmount, { from: vmAddress });
       await fiatToken.blacklist(communityFund, { from: blacklister });
@@ -364,15 +345,6 @@ contract("MockFiatTokenCeloWithCustomVmAddress", (accounts) => {
 
     const validTransferAmt = 1e4;
 
-    it("should fail when caller is not VM address", async () => {
-      await expectRevert(
-        fiatToken.internal_transferReservedGas(from, vmAddress, 10, {
-          from: from,
-        }),
-        onlyVmError
-      );
-    });
-
     it("should fail when 0 reserved gas is requested", async () => {
       await expectRevert(
         fiatToken.internal_transferReservedGas(from, vmAddress, 0, {