From 4641762a44d35e4b3b9d5daa73df2efe6708d124 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sat, 13 Jul 2019 10:46:27 +0200 Subject: [PATCH 1/4] SafeERC20: add extra returndatasize check to staticInvoke() --- contracts/common/SafeERC20.sol | 11 +- ...kenBalanceOfAllowanceMissingReturnMock.sol | 106 ++++++++++++++++++ test/contracts/common/safe_erc20.js | 19 ++++ 3 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 contracts/test/mocks/lib/token/TokenBalanceOfAllowanceMissingReturnMock.sol diff --git a/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol index fa11c0921..a009fae33 100644 --- a/contracts/common/SafeERC20.sol +++ b/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); diff --git a/contracts/test/mocks/lib/token/TokenBalanceOfAllowanceMissingReturnMock.sol b/contracts/test/mocks/lib/token/TokenBalanceOfAllowanceMissingReturnMock.sol new file mode 100644 index 000000000..9d32d2c1e --- /dev/null +++ b/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; + } +} diff --git a/test/contracts/common/safe_erc20.js b/test/contracts/common/safe_erc20.js index 00c08a311..d626e83ae 100644 --- a/test/contracts/common/safe_erc20.js +++ b/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`) @@ -122,4 +125,20 @@ contract('SafeERC20', ([owner, receiver]) => { }) }) } + + 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) + }) + }) }) From fbf0fad555d39e1e7bc98dccd364c67e3c5c3361 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 15 Jul 2019 17:22:58 +0200 Subject: [PATCH 2/4] chore: upgrade solidity-coverage to v0.6+ (#541) --- .solcover.js | 1 - package.json | 2 +- test/contracts/apps/app_funds.js | 2 +- test/contracts/apps/recovery_to_vault.js | 30 ++++++++---------------- test/contracts/kernel/kernel_funds.js | 2 +- test/helpers/assertThrow.js | 2 +- test/helpers/coverage.js | 14 ----------- 7 files changed, 14 insertions(+), 39 deletions(-) delete mode 100644 test/helpers/coverage.js diff --git a/.solcover.js b/.solcover.js index 38a69357e..706360613 100644 --- a/.solcover.js +++ b/.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 ] diff --git a/package.json b/package.json index ca57aac09..5bea6fdc1 100644 --- a/package.json +++ b/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", diff --git a/test/contracts/apps/app_funds.js b/test/contracts/apps/app_funds.js index 623736272..f94433287 100644 --- a/test/contracts/apps/app_funds.js +++ b/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') diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index 6f5e2010c..626bc67de 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/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 }) - })) + }) }) }) diff --git a/test/contracts/kernel/kernel_funds.js b/test/contracts/kernel/kernel_funds.js index 8a6c044a1..de80d198e 100644 --- a/test/contracts/kernel/kernel_funds.js +++ b/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') diff --git a/test/helpers/assertThrow.js b/test/helpers/assertThrow.js index c15565328..e70f6c26e 100644 --- a/test/helpers/assertThrow.js +++ b/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.`) } }, diff --git a/test/helpers/coverage.js b/test/helpers/coverage.js deleted file mode 100644 index f5f313242..000000000 --- a/test/helpers/coverage.js +++ /dev/null @@ -1,14 +0,0 @@ -const skipCoverage = test => { - // Required dynamic this binding to attach onto the running test - return function skipCoverage() { - if (process.env.SOLIDITY_COVERAGE === 'true') { - this.skip() - } else { - return test() - } - } -} - -module.exports = { - skipCoverage -} From 4feb97afc8f23b1bf5f89b2a3c8bb18f9c8f8923 Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Mon, 15 Jul 2019 13:05:59 -0300 Subject: [PATCH 3/4] SafeERC20: Add safe total supply (#543) --- contracts/common/SafeERC20.sol | 13 +++++++++++++ contracts/test/mocks/lib/token/SafeERC20Mock.sol | 4 ++++ test/contracts/common/safe_erc20.js | 6 ++++++ 3 files changed, 23 insertions(+) diff --git a/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol index fa11c0921..9a6afd27b 100644 --- a/contracts/common/SafeERC20.sol +++ b/contracts/common/SafeERC20.sol @@ -153,4 +153,17 @@ library SafeERC20 { return allowance; } + + /** + * @dev Static call into ERC20.totalSupply(). + * Reverts if the call fails for some reason (should never fail). + */ + 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; + } } diff --git a/contracts/test/mocks/lib/token/SafeERC20Mock.sol b/contracts/test/mocks/lib/token/SafeERC20Mock.sol index 9f13848f6..d1f83369d 100644 --- a/contracts/test/mocks/lib/token/SafeERC20Mock.sol +++ b/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(); + } } diff --git a/test/contracts/common/safe_erc20.js b/test/contracts/common/safe_erc20.js index 00c08a311..2b8d58505 100644 --- a/test/contracts/common/safe_erc20.js +++ b/test/contracts/common/safe_erc20.js @@ -120,6 +120,12 @@ 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") + }) }) } }) From ec1e0f5e84ab3fe308e7d6a0d0a8fc1008b84ff6 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 15 Jul 2019 18:07:07 +0200 Subject: [PATCH 4/4] SafeERC20: update revert comments for statically invoked functions --- contracts/common/SafeERC20.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol index 3897c5303..58ab01e7b 100644 --- a/contracts/common/SafeERC20.sol +++ b/contracts/common/SafeERC20.sol @@ -132,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( @@ -148,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( @@ -165,7 +165,7 @@ library SafeERC20 { /** * @dev Static call into ERC20.totalSupply(). - * 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 staticTotalSupply(ERC20 _token) internal view returns (uint256) { bytes memory totalSupplyCallData = abi.encodeWithSelector(_token.totalSupply.selector);