diff --git a/.solcover.js b/.solcover.js index 706360613..38a69357e 100644 --- a/.solcover.js +++ b/.solcover.js @@ -3,6 +3,7 @@ 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/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol new file mode 100644 index 000000000..fa11c0921 --- /dev/null +++ b/contracts/common/SafeERC20.sol @@ -0,0 +1,156 @@ +// Inspired by AdEx (https://github.com/AdExNetwork/adex-protocol-eth/blob/b9df617829661a7518ee10f4cb6c4108659dd6d5/contracts/libs/SafeERC20.sol) +// and 0x (https://github.com/0xProject/0x-monorepo/blob/737d1dc54d72872e24abce5a1dbe1b66d35fa21a/contracts/protocol/contracts/protocol/AssetProxy/ERC20Proxy.sol#L143) + +pragma solidity ^0.4.24; + +import "../lib/token/ERC20.sol"; + + +library SafeERC20 { + // Before 0.5, solidity has a mismatch between `address.transfer()` and `token.transfer()`: + // https://github.com/ethereum/solidity/issues/3544 + bytes4 private constant TRANSFER_SELECTOR = 0xa9059cbb; + + string private constant ERROR_TOKEN_BALANCE_REVERTED = "SAFE_ERC_20_BALANCE_REVERTED"; + string private constant ERROR_TOKEN_ALLOWANCE_REVERTED = "SAFE_ERC_20_ALLOWANCE_REVERTED"; + + function invokeAndCheckSuccess(address _addr, bytes memory _calldata) + private + returns (bool) + { + bool ret; + assembly { + let ptr := mload(0x40) // free memory pointer + + let success := call( + gas, // forward all gas + _addr, // address + 0, // no value + add(_calldata, 0x20), // calldata start + mload(_calldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + // Check number of bytes returned from last function call + switch returndatasize + + // No bytes returned: assume success + case 0 { + ret := 1 + } + + // 32 bytes returned: check if non-zero + case 0x20 { + // Only return success if returned data was true + // Already have output in ptr + ret := eq(mload(ptr), 1) + } + + // Not sure what was returned: don't mark as success + default { } + } + } + return ret; + } + + function staticInvoke(address _addr, bytes memory _calldata) + private + view + returns (bool, uint256) + { + bool success; + uint256 ret; + assembly { + let ptr := mload(0x40) // free memory pointer + + success := staticcall( + gas, // forward all gas + _addr, // address + add(_calldata, 0x20), // calldata start + mload(_calldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + ret := mload(ptr) + } + } + return (success, ret); + } + + /** + * @dev Same as a standards-compliant ERC20.transfer() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeTransfer(ERC20 _token, address _to, uint256 _amount) internal returns (bool) { + bytes memory transferCallData = abi.encodeWithSelector( + TRANSFER_SELECTOR, + _to, + _amount + ); + return invokeAndCheckSuccess(_token, transferCallData); + } + + /** + * @dev Same as a standards-compliant ERC20.transferFrom() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeTransferFrom(ERC20 _token, address _from, address _to, uint256 _amount) internal returns (bool) { + bytes memory transferFromCallData = abi.encodeWithSelector( + _token.transferFrom.selector, + _from, + _to, + _amount + ); + return invokeAndCheckSuccess(_token, transferFromCallData); + } + + /** + * @dev Same as a standards-compliant ERC20.approve() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeApprove(ERC20 _token, address _spender, uint256 _amount) internal returns (bool) { + bytes memory approveCallData = abi.encodeWithSelector( + _token.approve.selector, + _spender, + _amount + ); + return invokeAndCheckSuccess(_token, approveCallData); + } + + /** + * @dev Static call into ERC20.balanceOf(). + * Reverts if the call fails for some reason (should never fail). + */ + function staticBalanceOf(ERC20 _token, address _owner) internal view returns (uint256) { + bytes memory balanceOfCallData = abi.encodeWithSelector( + _token.balanceOf.selector, + _owner + ); + + (bool success, uint256 tokenBalance) = staticInvoke(_token, balanceOfCallData); + require(success, ERROR_TOKEN_BALANCE_REVERTED); + + return tokenBalance; + } + + /** + * @dev Static call into ERC20.allowance(). + * Reverts if the call fails for some reason (should never fail). + */ + function staticAllowance(ERC20 _token, address _owner, address _spender) internal view returns (uint256) { + bytes memory allowanceCallData = abi.encodeWithSelector( + _token.allowance.selector, + _owner, + _spender + ); + + (bool success, uint256 allowance) = staticInvoke(_token, allowanceCallData); + require(success, ERROR_TOKEN_ALLOWANCE_REVERTED); + + return allowance; + } +} diff --git a/contracts/common/VaultRecoverable.sol b/contracts/common/VaultRecoverable.sol index c298d66b4..810ac0cb3 100644 --- a/contracts/common/VaultRecoverable.sol +++ b/contracts/common/VaultRecoverable.sol @@ -8,11 +8,15 @@ import "../lib/token/ERC20.sol"; import "./EtherTokenConstant.sol"; import "./IsContract.sol"; import "./IVaultRecoverable.sol"; +import "./SafeERC20.sol"; contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { + using SafeERC20 for ERC20; + string private constant ERROR_DISALLOWED = "RECOVER_DISALLOWED"; string private constant ERROR_VAULT_NOT_CONTRACT = "RECOVER_VAULT_NOT_CONTRACT"; + string private constant ERROR_TOKEN_TRANSFER_FAILED = "RECOVER_TOKEN_TRANSFER_FAILED"; /** * @notice Send funds to recovery Vault. This contract should never receive funds, @@ -27,8 +31,9 @@ contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { if (_token == ETH) { vault.transfer(address(this).balance); } else { - uint256 amount = ERC20(_token).balanceOf(this); - ERC20(_token).transfer(vault, amount); + ERC20 token = ERC20(_token); + uint256 amount = token.staticBalanceOf(this); + require(token.safeTransfer(vault, amount), ERROR_TOKEN_TRANSFER_FAILED); } } diff --git a/contracts/test/mocks/SafeERC20Mock.sol b/contracts/test/mocks/SafeERC20Mock.sol new file mode 100644 index 000000000..92b660ced --- /dev/null +++ b/contracts/test/mocks/SafeERC20Mock.sol @@ -0,0 +1,36 @@ +pragma solidity 0.4.24; + +import "../../common/SafeERC20.sol"; +import "../../lib/token/ERC20.sol"; + + +contract SafeERC20Mock { + using SafeERC20 for ERC20; + event Result(bool result); + + function transfer(ERC20 token, address to, uint256 amount) external returns (bool) { + bool result = token.safeTransfer(to, amount); + emit Result(result); + return result; + } + + function transferFrom(ERC20 token, address from, address to, uint256 amount) external returns (bool) { + bool result = token.safeTransferFrom(from, to, amount); + emit Result(result); + return result; + } + + function approve(ERC20 token, address spender, uint256 amount) external returns (bool) { + bool result = token.safeApprove(spender, amount); + emit Result(result); + return result; + } + + function allowance(ERC20 token, address owner, address spender) external view returns (uint256) { + return token.staticAllowance(owner, spender); + } + + function balanceOf(ERC20 token, address owner) external view returns (uint256) { + return token.staticBalanceOf(owner); + } +} diff --git a/contracts/test/mocks/TokenMock.sol b/contracts/test/mocks/TokenMock.sol index 7442d80be..a93f777aa 100644 --- a/contracts/test/mocks/TokenMock.sol +++ b/contracts/test/mocks/TokenMock.sol @@ -1,4 +1,4 @@ -// Stripped from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol pragma solidity 0.4.24; @@ -8,14 +8,18 @@ import "../../lib/math/SafeMath.sol"; contract TokenMock { 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_; } @@ -29,12 +33,31 @@ contract TokenMock { return balances[_owner]; } + /** + * @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 returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @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)); @@ -43,4 +66,41 @@ contract TokenMock { 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/contracts/test/mocks/TokenReturnFalseMock.sol b/contracts/test/mocks/TokenReturnFalseMock.sol new file mode 100644 index 000000000..6011fc033 --- /dev/null +++ b/contracts/test/mocks/TokenReturnFalseMock.sol @@ -0,0 +1,110 @@ +// Standards compliant token that is returns false instead of reverting for +// `transfer()`, `transferFrom()`, and `approve(). +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol + +pragma solidity 0.4.24; + + +contract TokenReturnFalseMock { + 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 returns (uint256) { + return balances[_owner]; + } + + /** + * @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 returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @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) { + if (!allowTransfer_ || _to == address(0) || _value > balances[msg.sender]) { + return false; + } + + balances[msg.sender] = balances[msg.sender] - _value; + balances[_to] = balances[_to] + _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 + if (allowed[msg.sender][_spender] != 0) { + return false; + } + + 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) { + if (!allowTransfer_ || + _to == address(0) || + _value > balances[_from] || + _value > allowed[_from][msg.sender] + ) { + return false; + } + + balances[_from] = balances[_from] - _value; + balances[_to] = balances[_to] + _value; + allowed[_from][msg.sender] = allowed[_from][msg.sender] - _value; + emit Transfer(_from, _to, _value); + return true; + } +} diff --git a/contracts/test/mocks/TokenReturnMissingMock.sol b/contracts/test/mocks/TokenReturnMissingMock.sol new file mode 100644 index 000000000..8b339b1c8 --- /dev/null +++ b/contracts/test/mocks/TokenReturnMissingMock.sol @@ -0,0 +1,105 @@ +// Non-standards compliant token that is missing return values for +// `transfer()`, `transferFrom()`, and `approve(). +// 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 TokenReturnMissingMock { + 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 returns (uint256) { + return balances[_owner]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + + /** + * @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 returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @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 { + 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); + } + + /** + * @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 { + // 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); + } + + /** + * @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 { + 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); + } +} diff --git a/test/proxy_recover_funds.js b/test/recovery_to_vault.js similarity index 65% rename from test/proxy_recover_funds.js rename to test/recovery_to_vault.js index 94bdddd9a..678c90fd0 100644 --- a/test/proxy_recover_funds.js +++ b/test/recovery_to_vault.js @@ -14,6 +14,8 @@ const AppStubDepositable = artifacts.require('AppStubDepositable') const AppStubConditionalRecovery = artifacts.require('AppStubConditionalRecovery') const EtherTokenConstantMock = artifacts.require('EtherTokenConstantMock') const TokenMock = artifacts.require('TokenMock') +const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock') +const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock') const VaultMock = artifacts.require('VaultMock') const KernelDepositableMock = artifacts.require('KernelDepositableMock') @@ -22,34 +24,70 @@ const getEvent = (receipt, event, arg) => { return receipt.logs.filter(l => l.ev const APP_ID = hash('stub.aragonpm.test') const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies -contract('Proxy funds', accounts => { +contract('Recovery to vault', accounts => { let aclBase, appBase, appConditionalRecoveryBase let APP_ADDR_NAMESPACE, ETH const permissionsRoot = accounts[0] // Helpers - const recoverEth = async (target, vault) => { + const recoverEth = async ({ shouldFail, target, vault }) => { const amount = 1 const initialBalance = await getBalance(target.address) const initialVaultBalance = await getBalance(vault.address) const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) - await target.transferToVault(ETH) - assert.equal((await getBalance(target.address)).valueOf(), 0) - assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount).valueOf()) + + const recoverAction = () => target.transferToVault(ETH) + + if (shouldFail) { + await assertRevert(recoverAction) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance) + } else { + await recoverAction() + assert.equal((await getBalance(target.address)).valueOf(), 0) + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount)) + } + } + + const recoverTokens = async ({ shouldFail, tokenContract, target, vault }) => { + const amount = 1 + const token = await tokenContract.new(accounts[0], 1000) + const initialBalance = await token.balanceOf(target.address) + const initialVaultBalance = await token.balanceOf(vault.address) + await token.transfer(target.address, amount) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + + const recoverAction = () => target.transferToVault(token.address) + + if (shouldFail) { + await assertRevert(recoverAction) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance) + } else { + await recoverAction() + assert.equal((await token.balanceOf(target.address)).valueOf(), 0) + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount)) + } } - const recoverTokens = async (target, vault) => { + const failingRecoverTokens = async ({ tokenContract, target, vault }) => { const amount = 1 - const token = await TokenMock.new(accounts[0], 1000) + const token = await tokenContract.new(accounts[0], 1000) const initialBalance = await token.balanceOf(target.address) const initialVaultBalance = await token.balanceOf(vault.address) await token.transfer(target.address, amount) assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) - await target.transferToVault(token.address) - assert.equal((await token.balanceOf(target.address)).valueOf(), 0) - assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount).valueOf()) + + // Stop token from being transferable + await token.setAllowTransfer(false) + + // Try to transfer + await assertRevert(() => target.transferToVault(token.address)) + + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance) } const failWithoutVault = async (target, kernel) => { @@ -64,6 +102,22 @@ contract('Proxy funds', accounts => { }) } + // Token test groups + const tokenTestGroups = [ + { + title: 'standards compliant, reverting token', + tokenContract: TokenMock, + }, + { + title: 'standards compliant, non-reverting token', + tokenContract: TokenReturnFalseMock, + }, + { + title: 'non-standards compliant, missing return token', + tokenContract: TokenReturnMissingMock, + }, + ] + // Initial setup before(async () => { aclBase = await ACL.new() @@ -141,9 +195,25 @@ contract('Proxy funds', accounts => { ) ) - it('kernel recovers tokens', async () => { - await recoverTokens(kernel, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`kernel recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + vault, + target: kernel + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`kernel reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + vault, + target: kernel, + }) + }) + } context('> App without kernel', () => { beforeEach(async () => { @@ -152,15 +222,16 @@ contract('Proxy funds', accounts => { }) it('does not recover ETH', skipCoverageIfVaultProxy(async () => - await assertRevert( - () => recoverEth(target, vault) - ) + await recoverEth({ target, vault, shouldFail: true }) )) it('does not recover tokens', async () => - await assertRevert( - () => recoverTokens(target, vault) - ) + await recoverTokens({ + target, + vault, + shouldFail: true, + tokenContract: TokenMock, + }) ) }) @@ -187,13 +258,29 @@ contract('Proxy funds', accounts => { }) }) - it('recovers ETH', skipCoverageIfVaultProxy( - async () => await recoverEth(target, vault) + it('recovers ETH', skipCoverageIfVaultProxy(async () => + await recoverEth({ target, vault }) )) - it('recovers tokens', async () => { - await recoverTokens(target, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + target, + vault, + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + target, + vault, + }) + }) + } it('fails if vault is not contract', async () => { await failWithoutVault(target, kernel) @@ -211,16 +298,30 @@ contract('Proxy funds', accounts => { target = app }) - it('does not allow recovering ETH', skipCoverageIfVaultProxy( + it('does not allow recovering ETH', skipCoverageIfVaultProxy(async () => // Conditional stub doesnt allow eth recoveries - () => assertRevert( - () => recoverEth(target, vault) - ) + await recoverEth({ target, vault, shouldFail: true }) )) - it('allows recovering tokens', async () => { - await recoverTokens(target, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`allows recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + target, + vault, + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + target, + vault, + }) + }) + } }) }) } @@ -255,7 +356,7 @@ contract('Proxy funds', accounts => { }) it('recovers ETH from the kernel', skipCoverage(async () => { - await recoverEth(kernel, vault) + await recoverEth({ target: kernel, vault }) })) }) }) diff --git a/test/safe_erc20.js b/test/safe_erc20.js new file mode 100644 index 000000000..7996f091e --- /dev/null +++ b/test/safe_erc20.js @@ -0,0 +1,130 @@ +const { assertRevert } = require('./helpers/assertThrow') + +// Mocks +const SafeERC20Mock = artifacts.require('SafeERC20Mock') +const TokenMock = artifacts.require('TokenMock') +const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock') +const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock') + +const assertMockResult = (receipt, result) => { + const events = receipt.logs.filter(x => x.event == 'Result') + const eventArg = events[0].args.result + assert.equal(eventArg, result, `Result not expected (got ${eventArg} instead of ${result})`) +} + +contract('SafeERC20', accounts => { + const [owner, receiver] = accounts + const initialBalance = 10000 + let safeERC20Mock + + before(async () => { + safeERC20Mock = await SafeERC20Mock.new() + }) + + const testGroups = [ + { + title: 'Standards compliant, reverting token', + tokenContract: TokenMock, + }, + { + title: 'Standards compliant, non-reverting token', + tokenContract: TokenReturnFalseMock, + }, + { + title: 'Non-standards compliant, missing return token', + tokenContract: TokenReturnMissingMock, + }, + ] + + for ({ title, tokenContract} of testGroups) { + context(`> ${title}`, () => { + let tokenMock + + beforeEach(async () => { + tokenMock = await tokenContract.new(owner, initialBalance) + }) + + it('can correctly transfer', async () => { + // Add balance to the mock + const transferAmount = 5000 + await tokenMock.transfer(safeERC20Mock.address, transferAmount) + + // Transfer it all back from the mock + const receipt = await safeERC20Mock.transfer(tokenMock.address, owner, transferAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should be correct') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should be correct') + }) + + it('detects failed transfer', async () => { + // Attempt transfer when mock has no balance + const receipt = await safeERC20Mock.transfer(tokenMock.address, owner, 1000) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should stay the same') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('can correctly approve', async () => { + const approvedAmount = 5000 + + // Create approval from the mock + const receipt = await safeERC20Mock.approve(tokenMock.address, receiver, approvedAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.allowance(safeERC20Mock.address, receiver)).valueOf(), approvedAmount, 'Allowance of receiver should be correct') + }) + + it('detects failed approve', async () => { + const preApprovedAmount = 5000 + + // Create pre-exisiting approval + await safeERC20Mock.approve(tokenMock.address, receiver, preApprovedAmount) + + // Attempt to create another approval without reseting it back to 0 + const receipt = await safeERC20Mock.approve(tokenMock.address, receiver, preApprovedAmount - 500) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.allowance(safeERC20Mock.address, receiver)).valueOf(), preApprovedAmount, 'Allowance of receiver should be the pre-existing value') + }) + + it('can correctly transferFrom', async () => { + // Create approval + const approvedAmount = 5000 + await tokenMock.approve(safeERC20Mock.address, approvedAmount) + + // Transfer to receiver through the mock + const receipt = await safeERC20Mock.transferFrom(tokenMock.address, owner, receiver, approvedAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance - approvedAmount, 'Balance of owner should be correct') + assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), approvedAmount, 'Balance of receiver should be correct') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('detects failed transferFrom', async () => { + // Attempt transfer to receiver through the mock when mock wasn't approved + const receipt = await safeERC20Mock.transferFrom(tokenMock.address, owner, receiver, 5000) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should stay the same') + assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), 0, 'Balance of receiver should stay the same') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('gives correct value with static allowance', async () => { + // Create approval + const approvedAmount = 5000 + await tokenMock.approve(safeERC20Mock.address, approvedAmount) + + const approval = (await safeERC20Mock.allowance(tokenMock.address, owner, safeERC20Mock.address)).valueOf() + assert.equal(approval, approvedAmount, 'Mock should return correct allowance') + assert.equal((await tokenMock.allowance(owner, safeERC20Mock.address)).valueOf(), approval, "Mock should match token contract's allowance") + }) + + it('gives correct value with static balanceOf', async () => { + const balance = (await safeERC20Mock.balanceOf(tokenMock.address, owner)).valueOf() + assert.equal(balance, initialBalance, 'Mock should return correct balance') + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), balance, "Mock should match token contract's balance") + }) + }) + } +}) diff --git a/test/time_helpers.js b/test/time_helpers.js index 828640260..599afe88b 100644 --- a/test/time_helpers.js +++ b/test/time_helpers.js @@ -1,4 +1,4 @@ -contract('TimeHelpers test', accounts => { +contract('TimeHelpers', accounts => { let timeHelpersMock before(async () => {