SafeERC20: add extra returndatasize check to staticInvoke()

.solcover.js

@@ -3,7 +3,6 @@ const skipFiles = [
   'test',
   'acl/ACLSyntaxSugar.sol',
   'common/DepositableStorage.sol', // Used in tests that send ETH
-  'common/SafeERC20.sol', // solidity-coverage fails on assembly if (https://github.com/sc-forks/solidity-coverage/issues/287)
   'common/UnstructuredStorage.sol' // Used in tests that send ETH
 ]
 

contracts/common/SafeERC20.sol

@@ -75,7 +75,16 @@ library SafeERC20 {
             )
 
             if gt(success, 0) {
-                ret := mload(ptr)
+                switch returndatasize
+
+                // 32 bytes returned; is valid return
+                case 0x20 {
+                    ret := mload(ptr)
+                }
+                // Else, mark call as failed
+                default {
+                    success := 0
+                }
             }
         }
         return (success, ret);
@@ -123,7 +132,7 @@ library SafeERC20 {
 
     /**
     * @dev Static call into ERC20.balanceOf().
-    * Reverts if the call fails for some reason (should never fail).
+    * Reverts if the call fails for some reason (should never fail for correctly implemented ERC20s).
     */
     function staticBalanceOf(ERC20 _token, address _owner) internal view returns (uint256) {
         bytes memory balanceOfCallData = abi.encodeWithSelector(
@@ -139,7 +148,7 @@ library SafeERC20 {
 
     /**
     * @dev Static call into ERC20.allowance().
-    * Reverts if the call fails for some reason (should never fail).
+    * Reverts if the call fails for some reason (should never fail for correctly implemented ERC20s).
     */
     function staticAllowance(ERC20 _token, address _owner, address _spender) internal view returns (uint256) {
         bytes memory allowanceCallData = abi.encodeWithSelector(
@@ -153,4 +162,17 @@ library SafeERC20 {
 
         return allowance;
     }
+
+    /**
+    * @dev Static call into ERC20.totalSupply().
+    * Reverts if the call fails for some reason (should never fail for correctly implemented ERC20s).
+    */
+    function staticTotalSupply(ERC20 _token) internal view returns (uint256) {
+        bytes memory totalSupplyCallData = abi.encodeWithSelector(_token.totalSupply.selector);
+
+        (bool success, uint256 totalSupply) = staticInvoke(_token, totalSupplyCallData);
+        require(success, ERROR_TOKEN_ALLOWANCE_REVERTED);
+
+        return totalSupply;
+    }
 }

contracts/test/mocks/lib/token/SafeERC20Mock.sol

@@ -33,4 +33,8 @@ contract SafeERC20Mock {
     function balanceOf(ERC20 token, address owner) external view returns (uint256) {
         return token.staticBalanceOf(owner);
     }
+
+    function totalSupply(ERC20 token) external view returns (uint256) {
+        return token.staticTotalSupply();
+    }
 }

contracts/test/mocks/lib/token/TokenBalanceOfAllowanceMissingReturnMock.sol

@@ -0,0 +1,106 @@
+// Non-standards compliant token that is missing return values for
+// `allowance()` and `balanceOf()`.
+// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol
+
+pragma solidity 0.4.24;
+
+import "../../../../lib/math/SafeMath.sol";
+
+
+contract TokenBalanceOfAllowanceReturnMissingMock {
+    using SafeMath for uint256;
+    mapping (address => uint256) private balances;
+    mapping (address => mapping (address => uint256)) private allowed;
+    uint256 private totalSupply_;
+    bool private allowTransfer_;
+
+    event Approval(address indexed owner, address indexed spender, uint256 value);
+    event Transfer(address indexed from, address indexed to, uint256 value);
+
+    // Allow us to set the inital balance for an account on construction
+    constructor(address initialAccount, uint256 initialBalance) public {
+        balances[initialAccount] = initialBalance;
+        totalSupply_ = initialBalance;
+        allowTransfer_ = true;
+    }
+
+    function totalSupply() public view returns (uint256) { return totalSupply_; }
+
+    /**
+    * @dev Gets the balance of the specified address.
+    * @param _owner The address to query the the balance of.
+    * @return An uint256 representing the amount owned by the passed address.
+    */
+    function balanceOf(address _owner) public view {
+    }
+
+    /**
+    * @dev Function to check the amount of tokens that an owner allowed to a spender.
+    * @param _owner address The address which owns the funds.
+    * @param _spender address The address which will spend the funds.
+    * @return A uint256 specifying the amount of tokens still available for the spender.
+    */
+    function allowance(address _owner, address _spender) public view {
+    }
+
+    /**
+    * @dev Set whether the token is transferable or not
+    * @param _allowTransfer Should token be transferable
+    */
+    function setAllowTransfer(bool _allowTransfer) public {
+        allowTransfer_ = _allowTransfer;
+    }
+
+    /**
+    * @dev Transfer token for a specified address
+    * @param _to The address to transfer to.
+    * @param _value The amount to be transferred.
+    */
+    function transfer(address _to, uint256 _value) public returns (bool) {
+        require(allowTransfer_);
+        require(_value <= balances[msg.sender]);
+        require(_to != address(0));
+
+        balances[msg.sender] = balances[msg.sender].sub(_value);
+        balances[_to] = balances[_to].add(_value);
+        emit Transfer(msg.sender, _to, _value);
+        return true;
+    }
+
+    /**
+    * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
+    * Beware that changing an allowance with this method brings the risk that someone may use both the old
+    * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this
+    * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards:
+    * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
+    * @param _spender The address which will spend the funds.
+    * @param _value The amount of tokens to be spent.
+    */
+    function approve(address _spender, uint256 _value) public returns (bool) {
+        // Assume we want to protect for the race condition
+        require(allowed[msg.sender][_spender] == 0);
+
+        allowed[msg.sender][_spender] = _value;
+        emit Approval(msg.sender, _spender, _value);
+        return true;
+    }
+
+    /**
+    * @dev Transfer tokens from one address to another
+    * @param _from address The address which you want to send tokens from
+    * @param _to address The address which you want to transfer to
+    * @param _value uint256 the amount of tokens to be transferred
+    */
+    function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
+        require(allowTransfer_);
+        require(_value <= balances[_from]);
+        require(_value <= allowed[_from][msg.sender]);
+        require(_to != address(0));
+
+        balances[_from] = balances[_from].sub(_value);
+        balances[_to] = balances[_to].add(_value);
+        allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value);
+        emit Transfer(_from, _to, _value);
+        return true;
+    }
+}

package.json

@@ -39,7 +39,7 @@
     "ethereumjs-abi": "^0.6.5",
     "ganache-cli": "^6.4.2",
     "mocha-lcov-reporter": "^1.3.0",
-    "solidity-coverage": "0.5.8",
+    "solidity-coverage": "^0.6.2",
     "solium": "^1.2.3",
     "truffle": "4.1.14",
     "truffle-bytecode-manager": "^1.1.1",

test/contracts/apps/app_funds.js

@@ -1,7 +1,7 @@
 const { hash } = require('eth-ens-namehash')
 const { onlyIf } = require('../../helpers/onlyIf')
-const { getBalance } = require('../../helpers/web3')
 const { assertRevert } = require('../../helpers/assertThrow')
+const { getBalance } = require('../../helpers/web3')
 
 const ACL = artifacts.require('ACL')
 const Kernel = artifacts.require('Kernel')

test/contracts/apps/recovery_to_vault.js

@@ -1,9 +1,8 @@
 const { hash } = require('eth-ens-namehash')
-const { getBalance } = require('../../helpers/web3')
-const { skipCoverage } = require('../../helpers/coverage')
+const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3)
 const { assertRevert } = require('../../helpers/assertThrow')
 const { getNewProxyAddress } = require('../../helpers/events')
-const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3)
+const { getBalance } = require('../../helpers/web3')
 
 const ACL = artifacts.require('ACL')
 const Kernel = artifacts.require('Kernel')
@@ -160,15 +159,6 @@ contract('Recovery to vault', ([permissionsRoot]) => {
 
       // Test both the Vault itself and when it's behind a proxy to make sure their recovery behaviours are the same
       for (const vaultType of ['Vault', 'VaultProxy']) {
-        const skipCoverageIfVaultProxy = test => {
-          // The VaultMock isn't instrumented during coverage, but the AppProxy is, and so
-          // transferring to the fallback fails when we're testing with the proxy.
-          // Native transfers (either .send() or .transfer()) fail under coverage because they're
-          // limited to 2.3k gas, and the injected instrumentation from coverage makes these
-          // operations cost more than that limit.
-          return vaultType === 'VaultProxy' ? skipCoverage(test) : test
-        }
-
         context(`> ${vaultType}`, () => {
           let target, vault
 
@@ -218,9 +208,9 @@ contract('Recovery to vault', ([permissionsRoot]) => {
               await target.enableDeposits()
             })
 
-            it('does not recover ETH', skipCoverageIfVaultProxy(async () =>
+            it('does not recover ETH', async () =>
               await recoverEth({ target, vault, shouldFail: true })
-            ))
+            )
 
             it('does not recover tokens', async () =>
               await recoverTokens({
@@ -251,9 +241,9 @@ contract('Recovery to vault', ([permissionsRoot]) => {
               await assertRevert(target.sendTransaction({ value: 1, data: '0x01', gas: SEND_ETH_GAS }))
             })
 
-            it('recovers ETH', skipCoverageIfVaultProxy(async () =>
+            it('recovers ETH', async () =>
               await recoverEth({ target, vault })
-            ))
+            )
 
             for (const { title, tokenContract } of tokenTestGroups) {
               it(`recovers ${title}`, async () => {
@@ -289,10 +279,10 @@ contract('Recovery to vault', ([permissionsRoot]) => {
               target = app
             })
 
-            it('does not allow recovering ETH', skipCoverageIfVaultProxy(async () =>
+            it('does not allow recovering ETH', async () =>
               // Conditional stub doesnt allow eth recoveries
               await recoverEth({ target, vault, shouldFail: true })
-            ))
+            )
 
             for (const { title, tokenContract } of tokenTestGroups) {
               it(`allows recovers ${title}`, async () => {
@@ -344,8 +334,8 @@ contract('Recovery to vault', ([permissionsRoot]) => {
       await kernel.setRecoveryVaultAppId(vaultId)
     })
 
-    it('recovers ETH from the kernel', skipCoverage(async () => {
+    it('recovers ETH from the kernel', async () => {
       await recoverEth({ target: kernel, vault })
-    }))
+    })
   })
 })

test/contracts/common/safe_erc20.js

@@ -1,10 +1,13 @@
+const { assertRevert } = require('../../helpers/assertThrow')
 const { getEventArgument } = require('../../helpers/events')
+const reverts = require('../../helpers/revertStrings')
 
 // Mocks
 const SafeERC20Mock = artifacts.require('SafeERC20Mock')
 const TokenMock = artifacts.require('TokenMock')
 const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock')
 const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock')
+const TokenBalanceOfAllowanceReturnMissingMock = artifacts.require('TokenBalanceOfAllowanceReturnMissingMock')
 
 const assertMockResult = (receipt, result) => assert.equal(getEventArgument(receipt, 'Result', 'result'), result, `result does not match`)
 
@@ -120,6 +123,28 @@ contract('SafeERC20', ([owner, receiver]) => {
         assert.equal(balance, initialBalance, 'Mock should return correct balance')
         assert.equal((await tokenMock.balanceOf(owner)).valueOf(), balance, "Mock should match token contract's balance")
       })
+
+      it('gives correct value with static totalSupply', async () => {
+        const totalSupply = (await safeERC20Mock.totalSupply(tokenMock.address)).valueOf()
+        assert.equal(totalSupply, initialBalance, 'Mock should return correct total supply')
+        assert.equal((await tokenMock.totalSupply()).valueOf(), totalSupply, "Mock should match token contract's total supply")
+      })
     })
   }
+
+  context(`> Non-standards compliant, missing allowance and balance of return token`, () => {
+    let tokenMock
+
+    beforeEach(async () => {
+      tokenMock = await TokenBalanceOfAllowanceReturnMissingMock.new(owner, initialBalance)
+    })
+
+    it('fails on static allowance', async () => {
+      await assertRevert(safeERC20Mock.allowance(tokenMock.address, owner, safeERC20Mock.address), reverts.SAFE_ERC_20_ALLOWANCE_REVERTED)
+    })
+
+    it('fails on static balanceOf', async () => {
+      await assertRevert(safeERC20Mock.balanceOf(tokenMock.address, owner), reverts.SAFE_ERC_20_BALANCE_REVERTED)
+    })
+  })
 })

test/contracts/kernel/kernel_funds.js

@@ -1,6 +1,6 @@
+const { assertRevert } = require('../../helpers/assertThrow')
 const { onlyIf } = require('../../helpers/onlyIf')
 const { getBalance } = require('../../helpers/web3')
-const { assertRevert } = require('../../helpers/assertThrow')
 
 const ACL = artifacts.require('ACL')
 const Kernel = artifacts.require('Kernel')

test/helpers/assertThrow.js

@@ -31,7 +31,7 @@ module.exports = {
       error.reason = error.message.replace(errorPrefix, '').trim()
     }
 
-    if (process.env.SOLIDITY_COVERAGE !== 'true' && reason) {
+    if (reason) {
       assert.equal(error.reason, reason, `Expected revert reason "${reason}" but failed with "${error.reason || 'no reason'}" instead.`)
     }
   },