compound-finance · kevincheng96 · Dec 12, 2022 · Dec 10, 2022 · Dec 11, 2022 · Dec 11, 2022
diff --git a/contracts/IERC20NonStandard.sol b/contracts/IERC20NonStandard.sol
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.15;
+
+/**
+ * @title IERC20NonStandard
+ * @dev Version of ERC20 with no return values for `transfer` and `transferFrom`
+ *  See https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
+ */
+interface IERC20NonStandard {
+    function approve(address spender, uint256 amount) external;
+    function transfer(address to, uint256 value) external;
+    function transferFrom(address from, address to, uint256 value) external;
+    function balanceOf(address account) external view returns (uint256);
+}
diff --git a/contracts/bulkers/BaseBulker.sol b/contracts/bulkers/BaseBulker.sol
@@ -2,7 +2,7 @@
 pragma solidity 0.8.15;
 
 import "../CometInterface.sol";
-import "../ERC20.sol";
+import "../IERC20NonStandard.sol";
 import "../IWETH9.sol";
 
 /**
@@ -21,10 +21,14 @@ interface IClaimable {
  * @dev Note: Only intended to be used on EVM chains that have a native token and wrapped native token that implements the IWETH interface
  */
 contract BaseBulker {
+    /** Custom events **/
+
+    event AdminTransferred(address indexed oldAdmin, address indexed newAdmin);
+
     /** General configuration constants **/
 
     /// @notice The admin of the Bulker contract
-    address public immutable admin;
+    address public admin;
 
     /// @notice The address of the wrapped representation of the chain's native asset
     address payable public immutable wrappedNativeToken;
@@ -53,6 +57,8 @@ contract BaseBulker {
 
     error InvalidArgument();
     error FailedToSendNativeToken();
+    error TransferInFailed();
+    error TransferOutFailed();
     error Unauthorized();
     error UnhandledAction();
 
@@ -77,11 +83,11 @@ contract BaseBulker {
      * @param recipient The address that will receive the swept funds
      * @param asset The address of the ERC-20 token to sweep
      */
-    function sweepToken(address recipient, ERC20 asset) external {
+    function sweepToken(address recipient, address asset) external {
         if (msg.sender != admin) revert Unauthorized();
 
-        uint256 balance = asset.balanceOf(address(this));
-        asset.transfer(recipient, balance);
+        uint256 balance = IERC20NonStandard(asset).balanceOf(address(this));
+        doTransferOut(asset, recipient, balance);
     }
 
     /**
@@ -96,6 +102,17 @@ contract BaseBulker {
         if (!success) revert FailedToSendNativeToken();
     }
 
+    /**
+     * @notice Transfers the admin rights to a new address
+     */
+    function transferAdmin(address newAdmin) external {
+        if (msg.sender != admin) revert Unauthorized();
+
+        address oldAdmin = admin;
+        admin = newAdmin;
+        emit AdminTransferred(oldAdmin, newAdmin);
+    }
+
     /**
      * @notice Executes a list of actions in order
      * @param actions The list of actions to execute in order
@@ -197,4 +214,59 @@ contract BaseBulker {
     function claimReward(address comet, address rewards, address src, bool shouldAccrue) internal {
         IClaimable(rewards).claim(comet, src, shouldAccrue);
     }
+
+    /**
+     * @notice Similar to ERC-20 transfer, except it properly handles `transferFrom` from non-standard ERC-20 tokens
+     * @param asset The ERC-20 token to transfer in
+     * @param from The address to transfer from
+     * @param amount The amount of the token to transfer
+     * @dev Note: This does not check that the amount transferred in is actually equals to the amount specified (e.g. fee tokens will not revert)
+     * @dev Note: This wrapper safely handles non-standard ERC-20 tokens that do not return a value. See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
+     */
+    function doTransferIn(address asset, address from, uint amount) internal {
+        IERC20NonStandard(asset).transferFrom(from, address(this), amount);
+
+        bool success;
+        assembly {
+            switch returndatasize()
+                case 0 {                       // This is a non-standard ERC-20
+                    success := not(0)          // set success to true
+                }
+                case 32 {                      // This is a compliant ERC-20
+                    returndatacopy(0, 0, 32)
+                    success := mload(0)        // Set `success = returndata` of override external call
+                }
+                default {                      // This is an excessively non-compliant ERC-20, revert.
+                    revert(0, 0)
+                }
+        }
+        if (!success) revert TransferInFailed();
+    }
+
+    /**
+     * @notice Similar to ERC-20 transfer, except it properly handles `transfer` from non-standard ERC-20 tokens
+     * @param asset The ERC-20 token to transfer out
+     * @param to The recipient of the token transfer
+     * @param amount The amount of the token to transfer
+     * @dev Note: This wrapper safely handles non-standard ERC-20 tokens that do not return a value. See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
+     */
+    function doTransferOut(address asset, address to, uint amount) internal {
+        IERC20NonStandard(asset).transfer(to, amount);
+
+        bool success;
+        assembly {
+            switch returndatasize()
+                case 0 {                      // This is a non-standard ERC-20
+                    success := not(0)         // set success to true
+                }
+                case 32 {                     // This is a compliant ERC-20
+                    returndatacopy(0, 0, 32)
+                    success := mload(0)       // Set `success = returndata` of override external call
+                }
+                default {                     // This is an excessively non-compliant ERC-20, revert.
+                    revert(0, 0)
+                }
+        }
+        if (!success) revert TransferOutFailed();
+    }
 }
diff --git a/contracts/bulkers/MainnetBulker.sol b/contracts/bulkers/MainnetBulker.sol
@@ -61,7 +61,7 @@ contract MainnetBulker is BaseBulker {
      * @dev Note: This contract must have permission to manage msg.sender's Comet account
      */
     function supplyStEthTo(address comet, address to, uint stETHAmount) internal {
-        ERC20(steth).transferFrom(msg.sender, address(this), stETHAmount);
+        doTransferIn(steth, msg.sender, stETHAmount);
         ERC20(steth).approve(wsteth, stETHAmount);
         uint wstETHAmount = IWstETH(wsteth).wrap(stETHAmount);
         ERC20(wsteth).approve(comet, wstETHAmount);
@@ -75,6 +75,6 @@ contract MainnetBulker is BaseBulker {
     function withdrawStEthTo(address comet, address to, uint wstETHAmount) internal {
         CometInterface(comet).withdrawFrom(msg.sender, address(this), wsteth, wstETHAmount);
         uint stETHAmount = IWstETH(wsteth).unwrap(wstETHAmount);
-        ERC20(steth).transfer(to, stETHAmount);
+        doTransferOut(steth, to, stETHAmount);
     }
 }
diff --git a/contracts/test/NonStandardFaucetToken.sol b/contracts/test/NonStandardFaucetToken.sol
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.15;
+
+/**
+ * @title Non-standard ERC20 token
+ * @dev Implementation of the basic standard token.
+ *  See https://github.com/ethereum/EIPs/issues/20
+ * @dev Note: `transfer` and `transferFrom` do not return a boolean
+ */
+contract NonStandardToken {
+    string public name;
+    string public symbol;
+    uint8 public decimals;
+    uint256 public totalSupply;
+    mapping (address => mapping (address => uint256)) public allowance;
+    mapping(address => uint256) public balanceOf;
+    event Approval(address indexed owner, address indexed spender, uint256 value);
+    event Transfer(address indexed from, address indexed to, uint256 value);
+
+    constructor(uint256 _initialAmount, string memory _tokenName, uint8 _decimalUnits, string memory _tokenSymbol) {
+        totalSupply = _initialAmount;
+        balanceOf[msg.sender] = _initialAmount;
+        name = _tokenName;
+        symbol = _tokenSymbol;
+        decimals = _decimalUnits;
+    }
+
+    function transfer(address dst, uint256 amount) external virtual {
+        require(amount <= balanceOf[msg.sender], "ERC20: transfer amount exceeds balance");
+        balanceOf[msg.sender] = balanceOf[msg.sender] - amount;
+        balanceOf[dst] = balanceOf[dst] + amount;
+        emit Transfer(msg.sender, dst, amount);
+    }
+
+    function transferFrom(address src, address dst, uint256 amount) external virtual {
+        require(amount <= allowance[src][msg.sender], "ERC20: transfer amount exceeds allowance");
+        require(amount <= balanceOf[src], "ERC20: transfer amount exceeds balance");
+        allowance[src][msg.sender] = allowance[src][msg.sender] - amount;
+        balanceOf[src] = balanceOf[src] - amount;
+        balanceOf[dst] = balanceOf[dst] + amount;
+        emit Transfer(src, dst, amount);
+    }
+
+    function approve(address _spender, uint256 amount) external returns (bool) {
+        allowance[msg.sender][_spender] = amount;
+        emit Approval(msg.sender, _spender, amount);
+        return true;
+    }
+}
+
+/**
+ * @title The Compound Faucet Test Token
+ * @author Compound
+ * @notice A simple test token that lets anyone get more of it.
+ */
+contract NonStandardFaucetToken is NonStandardToken {
+    constructor(uint256 _initialAmount, string memory _tokenName, uint8 _decimalUnits, string memory _tokenSymbol)
+        NonStandardToken(_initialAmount, _tokenName, _decimalUnits, _tokenSymbol) {
+    }
+
+    function allocateTo(address _owner, uint256 value) public {
+        balanceOf[_owner] += value;
+        totalSupply += value;
+        emit Transfer(address(this), _owner, value);
+    }
+}
diff --git a/test/bulker-test.ts b/test/bulker-test.ts
@@ -1,5 +1,5 @@
-import { baseBalanceOf, ethers, expect, exp, makeProtocol, wait, makeBulker, defaultAssets, getGasUsed, makeRewards, fastForward } from './helpers';
-import { FaucetWETH__factory } from '../build/types';
+import { baseBalanceOf, ethers, expect, exp, makeProtocol, wait, makeBulker, defaultAssets, getGasUsed, makeRewards, fastForward, event } from './helpers';
+import { FaucetWETH__factory, NonStandardFaucetToken__factory } from '../build/types';
 
 // XXX Improve the "no permission" tests that should expect a custom error when
 // when https://github.com/nomiclabs/hardhat/issues/1618 gets fixed.
@@ -364,7 +364,38 @@ describe('bulker', function () {
   });
 
   describe('admin functions', function () {
-    it('sweep ERC20 token', async () => {
+    it('transferAdmin', async () => {
+      const protocol = await makeProtocol({});
+      const { governor, tokens: { WETH }, users: [alice] } = protocol;
+      const bulkerInfo = await makeBulker({ admin: governor, weth: WETH.address });
+      const { bulker } = bulkerInfo;
+
+      expect(await bulker.admin()).to.be.equal(governor.address);
+
+      // Admin transferred
+      const txn = await wait(bulker.connect(governor).transferAdmin(alice.address));
+
+      expect(event(txn, 0)).to.be.deep.equal({
+        AdminTransferred: {
+          oldAdmin: governor.address,
+          newAdmin: alice.address
+        }
+      });
+      expect(await bulker.admin()).to.be.equal(alice.address);
+    });
+
+    it('revert is transferAdmin called by non-admin', async () => {
+      const protocol = await makeProtocol({});
+      const { governor, tokens: { WETH }, users: [alice] } = protocol;
+      const bulkerInfo = await makeBulker({ admin: governor, weth: WETH.address });
+      const { bulker } = bulkerInfo;
+
+      await expect(
+        bulker.connect(alice).transferAdmin(alice.address)
+      ).to.be.revertedWith("custom error 'Unauthorized()'");
+    });
+
+    it('sweep standard ERC20 token', async () => {
       const protocol = await makeProtocol({});
       const { governor, tokens: { USDC, WETH }, users: [alice] } = protocol;
       const bulkerInfo = await makeBulker({ admin: governor, weth: WETH.address });
@@ -388,6 +419,35 @@ describe('bulker', function () {
       expect(newGovBalance.sub(oldGovBalance)).to.be.equal(transferAmount);
     });
 
+    it('sweep non-standard ERC20 token', async () => {
+      const protocol = await makeProtocol({});
+      const { governor, tokens: { WETH }, users: [alice] } = protocol;
+      const bulkerInfo = await makeBulker({ admin: governor, weth: WETH.address });
+      const { bulker } = bulkerInfo;
+
+      // Deploy non-standard token
+      const factory = (await ethers.getContractFactory('NonStandardFaucetToken')) as NonStandardFaucetToken__factory;
+      const nonStandardToken = await factory.deploy(1000e6, 'Tether', 6, 'USDT');
+      await nonStandardToken.deployed();
+
+      // Alice "accidentally" sends 10 non-standard tokens to the Bulker
+      const transferAmount = exp(10, 6);
+      await nonStandardToken.allocateTo(alice.address, transferAmount);
+      await nonStandardToken.connect(alice).transfer(bulker.address, transferAmount);
+
+      const oldBulkerBalance = await nonStandardToken.balanceOf(bulker.address);
+      const oldGovBalance = await nonStandardToken.balanceOf(governor.address);
+
+      // Governor sweeps tokens
+      await bulker.connect(governor).sweepToken(governor.address, nonStandardToken.address);
+
+      const newBulkerBalance = await nonStandardToken.balanceOf(bulker.address);
+      const newGovBalance = await nonStandardToken.balanceOf(governor.address);
+
+      expect(newBulkerBalance.sub(oldBulkerBalance)).to.be.equal(-transferAmount);
+      expect(newGovBalance.sub(oldGovBalance)).to.be.equal(transferAmount);
+    });
+
     it('sweep native token', async () => {
       const protocol = await makeProtocol({});
       const { governor, tokens: { WETH }, users: [alice] } = protocol;