diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c40e6a6de..693d32bd91 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/contracts/libraries/Allowance.sol b/contracts/libraries/Allowance.sol new file mode 100644 index 0000000000..c3e5f62e5d --- /dev/null +++ b/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"); + } + } +} diff --git a/contracts/p0/RevenueTrader.sol b/contracts/p0/RevenueTrader.sol index 2f380927fe..a62cf8bd18 100644 --- a/contracts/p0/RevenueTrader.sol +++ b/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); } } diff --git a/contracts/p0/mixins/Trading.sol b/contracts/p0/mixins/Trading.sol index b49aee3f11..52fc993eea 100644 --- a/contracts/p0/mixins/Trading.sol +++ b/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; diff --git a/contracts/p1/RevenueTrader.sol b/contracts/p1/RevenueTrader.sol index fe7b409b50..43253c6b0e 100644 --- a/contracts/p1/RevenueTrader.sol +++ b/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); } diff --git a/contracts/p1/mixins/Trading.sol b/contracts/p1/mixins/Trading.sol index 863f7ab113..24b2044d38 100644 --- a/contracts/p1/mixins/Trading.sol +++ b/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; diff --git a/contracts/plugins/mocks/BadERC20.sol b/contracts/plugins/mocks/BadERC20.sol index 99c7791e84..11570a4e4c 100644 --- a/contracts/plugins/mocks/BadERC20.sol +++ b/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"); diff --git a/contracts/plugins/trading/GnosisTrade.sol b/contracts/plugins/trading/GnosisTrade.sol index e8413c5fea..9f52e6387a 100644 --- a/contracts/plugins/trading/GnosisTrade.sol +++ b/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, diff --git a/hardhat.config.ts b/hardhat.config.ts index 7b540748d4..9dddd006bf 100644 --- a/hardhat.config.ts +++ b/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, diff --git a/test/scenario/BadERC20.test.ts b/test/scenario/BadERC20.test.ts index 6874a021ca..220ed60d0f 100644 --- a/test/scenario/BadERC20.test.ts +++ b/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) + }) + }) + }) })