Max approval erc20s (#971)

Co-authored-by: Akshat Mittal <[email protected]>
Co-authored-by: Patrick McKelvy <[email protected]>

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+# 3.0.1
+
+### Upgrade steps
+
+Update `BackingManager`, both `RevenueTraders` (rTokenTrader/rsrTrader), and call `Broker.setBatchTradeImplementation()` passing in the new `GnosisTrade` address.
+
 # 3.0.0
 
 ### Upgrade Steps

contracts/libraries/Allowance.sol

@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: BlueOak-1.0.0
+pragma solidity 0.8.19;
+
+interface IERC20ApproveOnly {
+    function approve(address spender, uint256 value) external;
+
+    function allowance(address owner, address spender) external view returns (uint256);
+}
+
+library AllowanceLib {
+    /// An approve helper that:
+    ///   1. Sets initial allowance to 0
+    ///   2. Tries to set the provided allowance
+    ///   3. Falls back to setting a maximum allowance, if (2) fails
+    /// Context: Some new-age ERC20s think it's a good idea to revert for allowances
+    /// that are > 0 but < type(uint256).max.
+    function safeApproveFallbackToMax(
+        address tokenAddress,
+        address spender,
+        uint256 value
+    ) internal {
+        IERC20ApproveOnly token = IERC20ApproveOnly(tokenAddress);
+
+        // 1. Set initial allowance to 0
+        token.approve(spender, 0);
+        require(token.allowance(address(this), spender) == 0, "allowance not 0");
+
+        if (value == 0) return;
+
+        // 2. Try to set the provided allowance
+        bool success; // bool success = false;
+        try token.approve(spender, value) {
+            success = token.allowance(address(this), spender) == value;
+            // solhint-disable-next-line no-empty-blocks
+        } catch {}
+
+        // 3. Fall-back to setting a maximum allowance
+        if (!success) {
+            token.approve(spender, type(uint256).max);
+            require(token.allowance(address(this), spender) >= value, "allowance missing");
+        }
+    }
+}

contracts/p0/RevenueTrader.sol

@@ -130,6 +130,9 @@ contract RevenueTraderP0 is TradingP0, IRevenueTrader {
         uint256 bal = tokenToBuy.balanceOf(address(this));
         tokenToBuy.safeApprove(address(main.distributor()), 0);
         tokenToBuy.safeApprove(address(main.distributor()), bal);
+        // do not need to use AllowanceLib.safeApproveFallbackToCustom here because
+        // tokenToBuy can be assumed to be either RSR or the RToken
+
         main.distributor().distribute(tokenToBuy, bal);
     }
 }

contracts/p0/mixins/Trading.sol

@@ -6,6 +6,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "../../interfaces/IBroker.sol";
 import "../../interfaces/IMain.sol";
 import "../../interfaces/ITrade.sol";
+import "../../libraries/Allowance.sol";
 import "../../libraries/Fixed.sol";
 import "./Rewardable.sol";
 
@@ -68,8 +69,16 @@ abstract contract TradingP0 is RewardableP0, ITrading {
         IBroker broker = main.broker();
         assert(address(trades[req.sell.erc20()]) == address(0));
 
-        req.sell.erc20().safeApprove(address(broker), 0);
-        req.sell.erc20().safeApprove(address(broker), req.sellAmount);
+        // Set allowance via custom approval -- first sets allowance to 0, then sets allowance
+        // to either the requested amount or the maximum possible amount, if that fails.
+        //
+        // Context: wcUSDCv3 has a non-standard approve() function that reverts if the approve
+        // amount is > 0 and < type(uint256).max.
+        AllowanceLib.safeApproveFallbackToMax(
+            address(req.sell.erc20()),
+            address(broker),
+            req.sellAmount
+        );
 
         trade = broker.openTrade(kind, req, prices);
         trades[req.sell.erc20()] = trade;

contracts/p1/RevenueTrader.sol

@@ -179,6 +179,9 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
         uint256 bal = tokenToBuy.balanceOf(address(this));
         tokenToBuy.safeApprove(address(distributor), 0);
         tokenToBuy.safeApprove(address(distributor), bal);
+
+        // do not need to use AllowanceLib.safeApproveFallbackToCustom here because
+        // tokenToBuy can be assumed to be either RSR or the RToken
         distributor.distribute(tokenToBuy, bal);
     }
 

contracts/p1/mixins/Trading.sol

@@ -7,6 +7,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@openzeppelin/contracts/utils/Multicall.sol";
 import "../../interfaces/ITrade.sol";
 import "../../interfaces/ITrading.sol";
+import "../../libraries/Allowance.sol";
 import "../../libraries/Fixed.sol";
 import "./Component.sol";
 import "./RewardableLib.sol";
@@ -120,8 +121,12 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl
         IERC20 sell = req.sell.erc20();
         assert(address(trades[sell]) == address(0));
 
-        IERC20Upgradeable(address(sell)).safeApprove(address(broker), 0);
-        IERC20Upgradeable(address(sell)).safeApprove(address(broker), req.sellAmount);
+        // Set allowance via custom approval -- first sets allowance to 0, then sets allowance
+        // to either the requested amount or the maximum possible amount, if that fails.
+        //
+        // Context: wcUSDCv3 has a non-standard approve() function that reverts if the approve
+        // amount is > 0 and < type(uint256).max.
+        AllowanceLib.safeApproveFallbackToMax(address(sell), address(broker), req.sellAmount);
 
         trade = broker.openTrade(kind, req, prices);
         trades[sell] = trade;

contracts/plugins/mocks/BadERC20.sol

@@ -11,6 +11,7 @@ contract BadERC20 is ERC20Mock {
     uint8 private _decimals;
     uint192 public transferFee; // {1}
     bool public revertDecimals;
+    bool public revertApprove; // if true, reverts for any approve > 0 and < type(uint256).max
 
     mapping(address => bool) public censored;
 
@@ -34,6 +35,10 @@ contract BadERC20 is ERC20Mock {
         censored[account] = val;
     }
 
+    function setRevertApprove(bool newRevertApprove) external {
+        revertApprove = newRevertApprove;
+    }
+
     function decimals() public view override returns (uint8) {
         bytes memory data = abi.encodePacked((bytes4(keccak256("absentDecimalsFn()"))));
 
@@ -42,6 +47,11 @@ contract BadERC20 is ERC20Mock {
         return _decimals;
     }
 
+    function approve(address spender, uint256 amount) public virtual override returns (bool) {
+        if (revertApprove && amount > 0 && amount < type(uint256).max) revert("revertApprove");
+        return super.approve(spender, amount);
+    }
+
     function transfer(address to, uint256 amount) public virtual override returns (bool) {
         address owner = _msgSender();
         if (censored[owner] || censored[to]) revert("censored");

contracts/plugins/trading/GnosisTrade.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.19;
 import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 import "@openzeppelin/contracts/utils/math/Math.sol";
 import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
+import "../../libraries/Allowance.sol";
 import "../../libraries/Fixed.sol";
 import "../../interfaces/IBroker.sol";
 import "../../interfaces/IGnosis.sol";
@@ -130,9 +131,12 @@ contract GnosisTrade is ITrade {
 
         // == Interactions ==
 
-        // Set allowance (two safeApprove calls to support USDT)
-        IERC20Upgradeable(address(sell)).safeApprove(address(gnosis), 0);
-        IERC20Upgradeable(address(sell)).safeApprove(address(gnosis), initBal);
+        // Set allowance via custom approval -- first sets allowance to 0, then sets allowance
+        // to either the requested amount or the maximum possible amount, if that fails.
+        //
+        // Context: wcUSDCv3 has a non-standard approve() function that reverts if the approve
+        // amount is > 0 and < type(uint256).max.
+        AllowanceLib.safeApproveFallbackToMax(address(sell), address(gnosis), initBal);
 
         auctionId = gnosis.initiateAuction(
             sell,

hardhat.config.ts

@@ -38,7 +38,7 @@ const config: HardhatUserConfig = {
       // network for tests/in-process stuff
       forking: useEnv('FORK')
         ? {
-            url: forkRpcs[(useEnv('FORK_NETWORK') ?? 'mainnet') as Network],
+            url: forkRpcs[useEnv('FORK_NETWORK', 'mainnet') as Network],
             blockNumber: Number(useEnv(`FORK_BLOCK`, forkBlockNumber['default'].toString())),
           }
         : undefined,

test/scenario/BadERC20.test.ts

@@ -396,4 +396,39 @@ describe(`Bad ERC20 - P${IMPLEMENTATION}`, () => {
         )
     })
   })
+
+  describe('with fussy approvals', function () {
+    let issueAmt: BigNumber
+
+    beforeEach(async () => {
+      issueAmt = initialBal.div(100)
+      await token0.connect(addr1).approve(rToken.address, issueAmt)
+      await token0.setRevertApprove(true)
+      await rToken.connect(addr1).issue(issueAmt)
+    })
+
+    context('Regression tests wcUSDCv3 10/10/2023', () => {
+      it('should not revert during recollateralization', async () => {
+        await basketHandler.setPrimeBasket(
+          [token0.address, backupToken.address],
+          [fp('0.5'), fp('0.5')]
+        )
+        await basketHandler.refreshBasket()
+
+        // Should launch recollateralization auction successfully
+        await expect(backingManager.rebalance(TradeKind.BATCH_AUCTION))
+          .to.emit(backingManager, 'TradeStarted')
+          .withArgs(anyValue, token0.address, backupToken.address, anyValue, anyValue)
+      })
+
+      it('should not revert during revenue auction', async () => {
+        await token0.mint(rsrTrader.address, issueAmt)
+
+        // Should launch revenue auction successfully
+        await expect(rsrTrader.manageTokens([token0.address], [TradeKind.BATCH_AUCTION]))
+          .to.emit(rsrTrader, 'TradeStarted')
+          .withArgs(anyValue, token0.address, rsr.address, anyValue, anyValue)
+      })
+    })
+  })
 })