From 647ecbb2470969ddc2fe9f2a15fe1aa08951b550 Mon Sep 17 00:00:00 2001 From: Oren Date: Sun, 13 Jan 2019 00:22:49 +0200 Subject: [PATCH 1/6] add SafeERC20 lib --- contracts/controller/Avatar.sol | 14 ++-- contracts/libs/SafeERC20.sol | 83 ++++++++++++++++++ contracts/schemes/Auction4Reputation.sol | 8 +- contracts/schemes/LockingToken4Reputation.sol | 15 ++-- contracts/test/BadERC20.sol | 22 +++++ contracts/test/SafeERC20Mock.sol | 39 +++++++++ .../universalSchemes/OrganizationRegister.sol | 5 +- contracts/universalSchemes/VestingScheme.sol | 48 +++++------ test/safeerc20.js | 73 ++++++++++++++++ test/vestingscheme.js | 84 +++++++++---------- 10 files changed, 305 insertions(+), 86 deletions(-) create mode 100644 contracts/libs/SafeERC20.sol create mode 100644 contracts/test/BadERC20.sol create mode 100644 contracts/test/SafeERC20Mock.sol create mode 100644 test/safeerc20.js diff --git a/contracts/controller/Avatar.sol b/contracts/controller/Avatar.sol index 50e456c5..ac26e3d9 100644 --- a/contracts/controller/Avatar.sol +++ b/contracts/controller/Avatar.sol @@ -4,14 +4,14 @@ import "@daostack/infra/contracts/Reputation.sol"; import "./DAOToken.sol"; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; -import "openzeppelin-solidity/contracts/token/ERC20/SafeERC20.sol"; +import "../libs/SafeERC20.sol"; /** * @title An Avatar holds tokens, reputation and ether for a controller */ contract Avatar is Ownable { - using SafeERC20 for IERC20; + using SafeERC20 for address; string public orgName; DAOToken public nativeToken; @@ -21,7 +21,7 @@ contract Avatar is Ownable { event SendEther(uint256 _amountInWei, address indexed _to); event ExternalTokenTransfer(address indexed _externalToken, address indexed _to, uint256 _value); event ExternalTokenTransferFrom(address indexed _externalToken, address _from, address _to, uint256 _value); - event ExternalTokenApproval(IERC20 indexed _externalToken, address _spender, uint256 _value); + event ExternalTokenApproval(address indexed _externalToken, address _spender, uint256 _value); event ReceiveEther(address indexed _sender, uint256 _value); /** @@ -79,7 +79,7 @@ contract Avatar is Ownable { function externalTokenTransfer(IERC20 _externalToken, address _to, uint256 _value) public onlyOwner returns(bool) { - _externalToken.safeTransfer(_to, _value); + address(_externalToken).safeTransfer(_to, _value); emit ExternalTokenTransfer(address(_externalToken), _to, _value); return true; } @@ -100,7 +100,7 @@ contract Avatar is Ownable { ) public onlyOwner returns(bool) { - _externalToken.safeTransferFrom(_from, _to, _value); + address(_externalToken).safeTransferFrom(_from, _to, _value); emit ExternalTokenTransferFrom(address(_externalToken), _from, _to, _value); return true; } @@ -116,8 +116,8 @@ contract Avatar is Ownable { function externalTokenApproval(IERC20 _externalToken, address _spender, uint256 _value) public onlyOwner returns(bool) { - require(_externalToken.approve(_spender, _value), "approve must succeed"); - emit ExternalTokenApproval(_externalToken, _spender, _value); + address(_externalToken).safeApprove(_spender, _value); + emit ExternalTokenApproval(address(_externalToken), _spender, _value); return true; } diff --git a/contracts/libs/SafeERC20.sol b/contracts/libs/SafeERC20.sol new file mode 100644 index 00000000..d478cf91 --- /dev/null +++ b/contracts/libs/SafeERC20.sol @@ -0,0 +1,83 @@ +/* + +badERC20 POC Fix by SECBIT Team + +USE WITH CAUTION & NO WARRANTY + +REFERENCE & RELATED READING +- https://github.com/ethereum/solidity/issues/4116 +- https://medium.com/@chris_77367/explaining-unexpected-reverts-starting-with-solidity-0-4-22-3ada6e82308c +- https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca +- https://gist.github.com/BrendanChou/88a2eeb80947ff00bcf58ffdafeaeb61 + +*/ +pragma solidity ^0.5.2; + +import "openzeppelin-solidity/contracts/utils/Address.sol"; +import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; + +library SafeERC20 { + using Address for address; + + function handleReturnData(bytes memory returnValue) internal pure returns (bool result) { + if (returnValue.length == 0) { + result = true; + } else if (returnValue.length == 32) { + // solhint-disable-next-line no-inline-assembly + assembly { + result := mload(add(returnValue, 32)) + } + } else { + revert(); + } + } + + function safeTransfer(address _erc20Addr, address _to, uint256 _value) internal { + + // Must be a contract addr first! + require(_erc20Addr.isContract()); + + // call return false when something wrong + (bool success, bytes memory returnValue) = + // solhint-disable-next-line avoid-low-level-calls + _erc20Addr.call(abi.encodeWithSignature("transfer(address,uint256)", _to, _value)); + require(success); + + // handle returndata + require(handleReturnData(returnValue)); + } + + function safeTransferFrom(address _erc20Addr, address _from, address _to, uint256 _value) internal { + + // Must be a contract addr first! + require(_erc20Addr.isContract()); + + // call return false when something wrong + (bool success, bytes memory returnValue) = + // solhint-disable-next-line avoid-low-level-calls + _erc20Addr.call(abi.encodeWithSignature("transferFrom(address,address,uint256)", _from, _to, _value)); + require(success); + + // handle returndata + require(handleReturnData(returnValue)); + } + + function safeApprove(address _erc20Addr, address _spender, uint256 _value) internal { + + // Must be a contract addr first! + require(_erc20Addr.isContract()); + + // safeApprove should only be called when setting an initial allowance, + // or when resetting it to zero. + require((_value == 0) || (IERC20(_erc20Addr).allowance(msg.sender, _spender) == 0)); + + // call return false when something wrong + (bool success, bytes memory returnValue) = + // solhint-disable-next-line avoid-low-level-calls + _erc20Addr.call(abi.encodeWithSignature("approve(address,uint256)", _spender, _value)); + require(success); + + // handle returndata + require(handleReturnData(returnValue)); + } +} diff --git a/contracts/schemes/Auction4Reputation.sol b/contracts/schemes/Auction4Reputation.sol index 5ae1d169..bd334acc 100644 --- a/contracts/schemes/Auction4Reputation.sol +++ b/contracts/schemes/Auction4Reputation.sol @@ -3,6 +3,7 @@ pragma solidity ^0.5.2; import "openzeppelin-solidity/contracts/math/SafeMath.sol"; import "../controller/ControllerInterface.sol"; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; +import "../libs/SafeERC20.sol"; /** * @title A scheme for conduct ERC20 Tokens auction for reputation @@ -11,6 +12,7 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; contract Auction4Reputation is Ownable { using SafeMath for uint256; + using SafeERC20 for address; event Bid(address indexed _bidder, uint256 indexed _auctionId, uint256 _amount); event Redeem(uint256 indexed _auctionId, address indexed _beneficiary, uint256 _amount); @@ -32,7 +34,7 @@ contract Auction4Reputation is Ownable { uint256 public auctionReputationReward; uint256 public auctionPeriod; uint256 public redeemEnableTime; - IERC20 public token; + IERC20 public token; address public wallet; /** @@ -116,7 +118,7 @@ contract Auction4Reputation is Ownable { require(now <= auctionsEndTime, "bidding should be within the allowed bidding period"); // solhint-disable-next-line not-rely-on-time require(now >= auctionsStartTime, "bidding is enable only after bidding auctionsStartTime"); - require(token.transferFrom(msg.sender, address(this), _amount), "transferFrom should succeed"); + address(token).safeTransferFrom(msg.sender, address(this), _amount); // solhint-disable-next-line not-rely-on-time auctionId = (now - auctionsStartTime) / auctionPeriod; Auction storage auction = auctions[auctionId]; @@ -143,7 +145,7 @@ contract Auction4Reputation is Ownable { // solhint-disable-next-line not-rely-on-time require(now > auctionsEndTime, "now > auctionsEndTime"); uint256 tokenBalance = token.balanceOf(address(this)); - require(token.transfer(wallet, tokenBalance), "transfer should succeed"); + address(token).safeTransfer(wallet, tokenBalance); } } diff --git a/contracts/schemes/LockingToken4Reputation.sol b/contracts/schemes/LockingToken4Reputation.sol index 7ea36eed..c6e81ae8 100644 --- a/contracts/schemes/LockingToken4Reputation.sol +++ b/contracts/schemes/LockingToken4Reputation.sol @@ -3,7 +3,7 @@ pragma solidity ^0.5.2; import "./Locking4Reputation.sol"; import "./PriceOracleInterface.sol"; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; -import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; +import "../libs/SafeERC20.sol"; /** @@ -11,10 +11,11 @@ import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; */ contract LockingToken4Reputation is Locking4Reputation, Ownable { + using SafeERC20 for address; PriceOracleInterface public priceOracleContract; // lockingId => token - mapping(bytes32 => IERC20) public lockedTokens; + mapping(bytes32 => address) public lockedTokens; event LockToken(bytes32 indexed _lockingId, address indexed _token, uint256 _numerator, uint256 _denominator); @@ -61,7 +62,7 @@ contract LockingToken4Reputation is Locking4Reputation, Ownable { */ function release(address _beneficiary, bytes32 _lockingId) public returns(bool) { uint256 amount = super._release(_beneficiary, _lockingId); - require(lockedTokens[_lockingId].transfer(_beneficiary, amount), "transfer should succeed"); + lockedTokens[_lockingId].safeTransfer(_beneficiary, amount); return true; } @@ -73,22 +74,22 @@ contract LockingToken4Reputation is Locking4Reputation, Ownable { * @param _token the token to lock - this should be whitelisted at the priceOracleContract * @return lockingId */ - function lock(uint256 _amount, uint256 _period, IERC20 _token) public returns(bytes32 lockingId) { + function lock(uint256 _amount, uint256 _period, address _token) public returns(bytes32 lockingId) { uint256 numerator; uint256 denominator; - (numerator, denominator) = priceOracleContract.getPrice(address(_token)); + (numerator, denominator) = priceOracleContract.getPrice(_token); require(numerator > 0, "numerator should be > 0"); require(denominator > 0, "denominator should be > 0"); - require(_token.transferFrom(msg.sender, address(this), _amount), "transferFrom should succeed"); + _token.safeTransferFrom(msg.sender, address(this), _amount); lockingId = super._lock(_amount, _period, msg.sender, numerator, denominator); lockedTokens[lockingId] = _token; - emit LockToken(lockingId, address(_token), numerator, denominator); + emit LockToken(lockingId, _token, numerator, denominator); } } diff --git a/contracts/test/BadERC20.sol b/contracts/test/BadERC20.sol new file mode 100644 index 00000000..66c59277 --- /dev/null +++ b/contracts/test/BadERC20.sol @@ -0,0 +1,22 @@ +pragma solidity ^0.5.2; + +//this is a mock to simulate bad ERC20 token implementation as describe at +//https://github.com/ethereum/solidity/issues/4116 +contract BadERC20 { + + mapping(address => uint256) public balances; + mapping (address => mapping (address => uint256)) public allowance; + + function transfer(address _to, uint256 _value) public { + balances[_to] = _value; + } + + function transferFrom(address, address _to, uint256 _value) public { + balances[_to] += _value; + } + + function approve(address _spender, uint256 _value) public { + allowance[msg.sender][_spender] = _value; + } + +} diff --git a/contracts/test/SafeERC20Mock.sol b/contracts/test/SafeERC20Mock.sol new file mode 100644 index 00000000..e26228de --- /dev/null +++ b/contracts/test/SafeERC20Mock.sol @@ -0,0 +1,39 @@ +pragma solidity ^0.5.2; + +import "./BadERC20.sol"; +import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; +import "../libs/SafeERC20.sol"; + + +contract SafeERC20Mock { + using SafeERC20 for address; + + address public token; + + constructor(IERC20 _token) public { + token = address(_token); + } + + function transfer(address _to, uint256 _value) public returns(bool) { + (, bytes memory returnValue) = + // solhint-disable-next-line avoid-low-level-calls + token.call(abi.encodeWithSignature("transfer(address,uint256)", _to, _value)); + require(returnValue.length > 0); + } + + function transferWithFix(address _to, uint256 _value) public returns(bool) { + token.safeTransfer(_to, _value); + return true; + } + + function transferFromWithFix(address _from, address _to, uint256 _value) public returns(bool) { + token.safeTransferFrom(_from, _to, _value); + return true; + } + + function approveWithFix(address _spender, uint256 _value) public returns(bool) { + token.safeApprove(_spender, _value); + return true; + } + +} diff --git a/contracts/universalSchemes/OrganizationRegister.sol b/contracts/universalSchemes/OrganizationRegister.sol index dd36c8a3..097e83a8 100644 --- a/contracts/universalSchemes/OrganizationRegister.sol +++ b/contracts/universalSchemes/OrganizationRegister.sol @@ -1,6 +1,7 @@ pragma solidity ^0.5.2; import "./UniversalScheme.sol"; +import "../libs/SafeERC20.sol"; /** * @title A universal organization registry. @@ -8,9 +9,9 @@ import "./UniversalScheme.sol"; * Other organizations can then add and promote themselves on this registry. */ - contract OrganizationRegister is UniversalScheme { using SafeMath for uint; + using SafeERC20 for address; struct Parameters { uint256 fee; @@ -72,7 +73,7 @@ contract OrganizationRegister is UniversalScheme { // Pay promotion, if the org was not listed the minimum is the fee: require((organizationsRegistry[address(_avatar)][_record] > 0) || (_amount >= params.fee)); - require(params.token.transferFrom(msg.sender, params.beneficiary, _amount)); + address(params.token).safeTransferFrom(msg.sender, params.beneficiary, _amount); if (organizationsRegistry[address(_avatar)][_record] == 0) { emit OrgAdded(address(_avatar), _record); } diff --git a/contracts/universalSchemes/VestingScheme.sol b/contracts/universalSchemes/VestingScheme.sol index 83436596..b6e20137 100644 --- a/contracts/universalSchemes/VestingScheme.sol +++ b/contracts/universalSchemes/VestingScheme.sol @@ -4,6 +4,7 @@ import "@daostack/infra/contracts/votingMachines/IntVoteInterface.sol"; import "@daostack/infra/contracts/votingMachines/VotingMachineCallbacksInterface.sol"; import "./UniversalScheme.sol"; import "../votingMachines/VotingMachineCallbacks.sol"; +import "../libs/SafeERC20.sol"; /** @@ -13,6 +14,7 @@ import "../votingMachines/VotingMachineCallbacks.sol"; contract VestingScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecuteInterface { using SafeMath for uint; + using SafeERC20 for address; event ProposalExecuted(address indexed _avatar, bytes32 indexed _proposalId, int256 _param); event ProposalDeleted(address indexed _avatar, bytes32 indexed _proposalId); @@ -163,12 +165,13 @@ contract VestingScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu * @param _token the relevant token in the agreement. * @param _beneficiary the beneficiary of the agreement. * @param _returnOnCancelAddress where to send the tokens in case of stoping. - * @param _startingBlock the block from which the agreement starts. - * @param _amountPerPeriod amount of tokens per period. - * @param _periodLength period length in blocks. - * @param _numOfAgreedPeriods how many periods agreed on. - * @param _cliffInPeriods the length of the cliff in periods. - * @param _signaturesReqToCancel number of signatures required to cancel agreement. + * @param _params array + * _params[0] _startingBlock the block from which the agreement starts. + * _params[1] _amountPerPeriod amount of tokens per period. + * _params[2] _periodLength period length in blocks. + * _params[3] _numOfAgreedPeriods how many periods agreed on. + * _params[4] _cliffInPeriods the length of the cliff in periods. + * _params[5] _signaturesReqToCancel number of signatures required to cancel agreement. * @param _signersArray avatar array of addresses that can sign to cancel agreement. * @return uint256 the agreement index. */ @@ -176,34 +179,29 @@ contract VestingScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu IERC20 _token, address _beneficiary, address _returnOnCancelAddress, - uint256 _startingBlock, - uint256 _amountPerPeriod, - uint256 _periodLength, - uint256 _numOfAgreedPeriods, - uint256 _cliffInPeriods, - uint256 _signaturesReqToCancel, + uint[6] calldata _params, address[] calldata _signersArray ) external returns(uint256) { - require(_signaturesReqToCancel <= _signersArray.length); - require(_periodLength > 0); - require(_numOfAgreedPeriods > 0, "Number of Agreed Periods must be greater than 0"); + require(_params[5] <= _signersArray.length); + require(_params[2] > 0); + require(_params[3] > 0, "Number of Agreed Periods must be greater than 0"); // Collect funds: - uint256 totalAmount = _amountPerPeriod.mul(_numOfAgreedPeriods); - require(_token.transferFrom(msg.sender, address(this), totalAmount)); + uint256 totalAmount = _params[1].mul(_params[3]); + address(_token).safeTransferFrom(msg.sender, address(this), totalAmount); // Write parameters: agreements[agreementsCounter].token = _token; agreements[agreementsCounter].beneficiary = _beneficiary; agreements[agreementsCounter].returnOnCancelAddress = _returnOnCancelAddress; - agreements[agreementsCounter].startingBlock = _startingBlock; - agreements[agreementsCounter].amountPerPeriod = _amountPerPeriod; - agreements[agreementsCounter].periodLength = _periodLength; - agreements[agreementsCounter].numOfAgreedPeriods = _numOfAgreedPeriods; - agreements[agreementsCounter].cliffInPeriods = _cliffInPeriods; - agreements[agreementsCounter].signaturesReqToCancel = _signaturesReqToCancel; + agreements[agreementsCounter].startingBlock = _params[0]; + agreements[agreementsCounter].amountPerPeriod = _params[1]; + agreements[agreementsCounter].periodLength = _params[2]; + agreements[agreementsCounter].numOfAgreedPeriods = _params[3]; + agreements[agreementsCounter].cliffInPeriods = _params[4]; + agreements[agreementsCounter].signaturesReqToCancel = _params[5]; // Write the signers mapping: for (uint256 cnt = 0; cnt < _signersArray.length; cnt++) { @@ -309,7 +307,7 @@ contract VestingScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu // Transfer: uint256 tokensToTransfer = periodsToPay.mul(agreement.amountPerPeriod); - require(agreement.token.transfer(agreement.beneficiary, tokensToTransfer)); + address(agreement.token).safeTransfer(agreement.beneficiary, tokensToTransfer); // Log collecting: emit Collect(_agreementId); @@ -324,7 +322,7 @@ contract VestingScheme is UniversalScheme, VotingMachineCallbacks, ProposalExecu delete agreements[_agreementId]; uint256 periodsLeft = agreement.numOfAgreedPeriods.sub(agreement.collectedPeriods); uint256 tokensLeft = periodsLeft.mul(agreement.amountPerPeriod); - require(agreement.token.transfer(agreement.returnOnCancelAddress, tokensLeft)); + address(agreement.token).safeTransfer(agreement.returnOnCancelAddress, tokensLeft); // Log canceling agreement: emit AgreementCancel(_agreementId); } diff --git a/test/safeerc20.js b/test/safeerc20.js new file mode 100644 index 00000000..f20f0e4a --- /dev/null +++ b/test/safeerc20.js @@ -0,0 +1,73 @@ +const helpers = require('./helpers'); +const SafeERC20Mock = artifacts.require('./test/SafeERC20Mock.sol'); +const BadERC20 = artifacts.require('./test/BadERC20.sol'); +const ERC20Mock = artifacts.require('./test/ERC20Mock.sol'); + + +contract('safe erc 20', accounts => { + + it("transfer bad token", async () => { + var badERC20 = await BadERC20.new(); + var safeERC20Mock = await SafeERC20Mock.new(badERC20.address); + assert.equal(await safeERC20Mock.transferWithFix.call(accounts[1],123),true); + await safeERC20Mock.transferWithFix(accounts[1],123); + assert.equal(await badERC20.balances(accounts[1]),123); + }); + + it("transfer bad token without the fix revert", async () => { + var badERC20 = await BadERC20.new(); + var safeERC20Mock = await SafeERC20Mock.new(badERC20.address); + try { + await safeERC20Mock.transfer(accounts[1],123); + + assert(false, "transfer bad token without the fix revert"); + } catch(error) { + helpers.assertVMException(error); + } + }); + + it("transfer good token", async () => { + var goodToken = await ERC20Mock.new(accounts[0], web3.utils.toWei('100', "ether")); + var safeERC20Mock = await SafeERC20Mock.new(goodToken.address); + await goodToken.transfer(safeERC20Mock.address,123); + assert.equal(await safeERC20Mock.transferWithFix.call(accounts[1],123,{from:accounts[0]}),true); + await safeERC20Mock.transferWithFix(accounts[1],123,{from:accounts[0]}); + assert.equal(await goodToken.balanceOf(accounts[1]),123); + }); + + + it("transferFrom bad token", async () => { + var badERC20 = await BadERC20.new(); + var safeERC20Mock = await SafeERC20Mock.new(badERC20.address); + assert.equal(await safeERC20Mock.transferFromWithFix.call(accounts[0],accounts[1],123),true); + await safeERC20Mock.transferFromWithFix(accounts[0],accounts[1],123); + assert.equal(await badERC20.balances(accounts[1]),123); + }); + + + it("transferFrom good token", async () => { + var goodToken = await ERC20Mock.new(accounts[0], web3.utils.toWei('100', "ether")); + var safeERC20Mock = await SafeERC20Mock.new(goodToken.address); + await goodToken.approve(safeERC20Mock.address,123); + assert.equal(await safeERC20Mock.transferFromWithFix.call(accounts[0],accounts[1],123,{from:accounts[0]}),true); + await safeERC20Mock.transferFromWithFix(accounts[0],accounts[1],123,{from:accounts[0]}); + assert.equal(await goodToken.balanceOf(accounts[1]),123); + }); + + it("approve bad token", async () => { + var badERC20 = await BadERC20.new(); + var safeERC20Mock = await SafeERC20Mock.new(badERC20.address); + assert.equal(await safeERC20Mock.approveWithFix.call(accounts[0],123),true); + await safeERC20Mock.approveWithFix(accounts[0],123); + assert.equal(await badERC20.allowance(safeERC20Mock.address,accounts[0]),123); + }); + + + it("approve good token", async () => { + var goodToken = await ERC20Mock.new(accounts[0], web3.utils.toWei('100', "ether")); + var safeERC20Mock = await SafeERC20Mock.new(goodToken.address); + assert.equal(await safeERC20Mock.approveWithFix.call(accounts[0],123),true); + await safeERC20Mock.approveWithFix(accounts[0],123); + assert.equal(await goodToken.allowance(safeERC20Mock.address,accounts[0]),123); + }); +}); diff --git a/test/vestingscheme.js b/test/vestingscheme.js index 2b0f2476..d24b97d8 100644 --- a/test/vestingscheme.js +++ b/test/vestingscheme.js @@ -272,12 +272,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - 0, + 0], [],{from:accounts[1]}); assert.equal(tx.logs.length, 1); assert.equal(tx.logs[0].event, "NewVestedAgreement"); @@ -288,12 +288,12 @@ contract('VestingScheme', accounts => { tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - 0, + 0], [], {from:accounts[1]}); assert.equal(tx.logs.length, 1); @@ -315,12 +315,12 @@ contract('VestingScheme', accounts => { await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, periodLength, numberOfAgreedPeriods, 11, - 0, + 0], [],{from:accounts[1]}); assert(false,"createVestedAgreement should fail - due to periodLength == 0"); @@ -339,12 +339,12 @@ contract('VestingScheme', accounts => { await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - 0, + 0], [],{from:accounts[1]}); assert.equal(await testSetup.standardTokenMock.balanceOf(testSetup.vestingScheme.address),amountPerPeriod*numberOfAgreedPeriods); }); @@ -366,12 +366,12 @@ contract('VestingScheme', accounts => { await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); assert(false,"createVestedAgreement should fail - due to _signaturesReqToCancel > _signersArray.length !"); }catch(ex){ @@ -390,12 +390,12 @@ contract('VestingScheme', accounts => { await testSetup.vestingScheme.createVestedAgreement(testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, periodLength, numberOfAgreedPeriods, 11, - 0, [], { + 0], [], { from: accounts[1] }); @@ -419,12 +419,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); assert.equal(agreementId,0); @@ -447,12 +447,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); try{ @@ -478,12 +478,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); try{ @@ -509,12 +509,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); await testSetup.vestingScheme.signToCancelAgreement(agreementId); @@ -542,12 +542,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], returnOnCancelAddress, - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); tx = await testSetup.vestingScheme.signToCancelAgreement(agreementId); @@ -580,12 +580,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); assert.equal(agreementId,0); @@ -618,12 +618,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); assert.equal(agreementId,0); @@ -654,12 +654,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); assert.equal(agreementId,0); @@ -691,12 +691,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, accounts[0], accounts[1], - blockNumber, + [blockNumber, amountPerPeriod, 2, numberOfAgreedPeriods, 11, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); assert.equal(agreementId,0); @@ -727,12 +727,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, beneficiary, accounts[1], - startingBlock, + [startingBlock, amountPerPeriod, periodLength, numberOfAgreedPeriods, cliffInPeriods, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); tx = await testSetup.vestingScheme.collect(agreementId,{from:beneficiary}); @@ -761,12 +761,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, beneficiary, accounts[1], - startingBlock, + [startingBlock, amountPerPeriod, periodLength, numberOfAgreedPeriods, cliffInPeriods, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); tx = await testSetup.vestingScheme.collect(agreementId,{from:beneficiary}); @@ -796,12 +796,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, beneficiary, accounts[1], - startingBlock, + [startingBlock, amountPerPeriod, periodLength, numberOfAgreedPeriods, cliffInPeriods, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); try{ @@ -830,12 +830,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, beneficiary, accounts[1], - startingBlock, + [startingBlock, amountPerPeriod, periodLength, numberOfAgreedPeriods, cliffInPeriods, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); try{ @@ -865,12 +865,12 @@ contract('VestingScheme', accounts => { var tx = await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, beneficiary, accounts[1], - startingBlock, + [startingBlock, amountPerPeriod, periodLength, numberOfAgreedPeriods, cliffInPeriods, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); var agreementId = await helpers.getValueFromLogs(tx, '_agreementId',1); await testSetup.vestingScheme.collect(agreementId,{from:beneficiary}); @@ -897,12 +897,12 @@ contract('VestingScheme', accounts => { await testSetup.vestingScheme.createVestedAgreement( testSetup.standardTokenMock.address, beneficiary, accounts[1], - startingBlock, + [startingBlock, amountPerPeriod, periodLength, numberOfAgreedPeriods, cliffInPeriods, - _signaturesReqToCancel, + _signaturesReqToCancel], _signersArray,{from:accounts[1]}); for (i=0;i Date: Sun, 13 Jan 2019 00:41:58 +0200 Subject: [PATCH 2/6] use encodeWithSelector --- contracts/libs/SafeERC20.sol | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/contracts/libs/SafeERC20.sol b/contracts/libs/SafeERC20.sol index d478cf91..28ff1681 100644 --- a/contracts/libs/SafeERC20.sol +++ b/contracts/libs/SafeERC20.sol @@ -19,6 +19,10 @@ import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol"; library SafeERC20 { using Address for address; + bytes4 constant private TRANSFER_SELECTOR = bytes4(keccak256(bytes("transfer(address,uint256)"))); + bytes4 constant private TRANSFERFROM_SELECTOR = bytes4(keccak256(bytes("transferFrom(address,address,uint256)"))); + bytes4 constant private APPROVE_SELECTOR = bytes4(keccak256(bytes("approve(address,uint256)"))); + function handleReturnData(bytes memory returnValue) internal pure returns (bool result) { if (returnValue.length == 0) { result = true; @@ -40,7 +44,7 @@ library SafeERC20 { // call return false when something wrong (bool success, bytes memory returnValue) = // solhint-disable-next-line avoid-low-level-calls - _erc20Addr.call(abi.encodeWithSignature("transfer(address,uint256)", _to, _value)); + _erc20Addr.call(abi.encodeWithSelector(TRANSFER_SELECTOR, _to, _value)); require(success); // handle returndata @@ -55,7 +59,7 @@ library SafeERC20 { // call return false when something wrong (bool success, bytes memory returnValue) = // solhint-disable-next-line avoid-low-level-calls - _erc20Addr.call(abi.encodeWithSignature("transferFrom(address,address,uint256)", _from, _to, _value)); + _erc20Addr.call(abi.encodeWithSelector(TRANSFERFROM_SELECTOR, _from, _to, _value)); require(success); // handle returndata @@ -74,7 +78,7 @@ library SafeERC20 { // call return false when something wrong (bool success, bytes memory returnValue) = // solhint-disable-next-line avoid-low-level-calls - _erc20Addr.call(abi.encodeWithSignature("approve(address,uint256)", _spender, _value)); + _erc20Addr.call(abi.encodeWithSelector(APPROVE_SELECTOR, _spender, _value)); require(success); // handle returndata From b7e6d5a3cc8dabdcbe8f41fec7daf0353b219d4a Mon Sep 17 00:00:00 2001 From: Oren Date: Sun, 13 Jan 2019 00:50:43 +0200 Subject: [PATCH 3/6] cleanup --- contracts/libs/SafeERC20.sol | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/contracts/libs/SafeERC20.sol b/contracts/libs/SafeERC20.sol index 28ff1681..c9999066 100644 --- a/contracts/libs/SafeERC20.sol +++ b/contracts/libs/SafeERC20.sol @@ -23,28 +23,15 @@ library SafeERC20 { bytes4 constant private TRANSFERFROM_SELECTOR = bytes4(keccak256(bytes("transferFrom(address,address,uint256)"))); bytes4 constant private APPROVE_SELECTOR = bytes4(keccak256(bytes("approve(address,uint256)"))); - function handleReturnData(bytes memory returnValue) internal pure returns (bool result) { - if (returnValue.length == 0) { - result = true; - } else if (returnValue.length == 32) { - // solhint-disable-next-line no-inline-assembly - assembly { - result := mload(add(returnValue, 32)) - } - } else { - revert(); - } - } - function safeTransfer(address _erc20Addr, address _to, uint256 _value) internal { // Must be a contract addr first! require(_erc20Addr.isContract()); - // call return false when something wrong (bool success, bytes memory returnValue) = // solhint-disable-next-line avoid-low-level-calls _erc20Addr.call(abi.encodeWithSelector(TRANSFER_SELECTOR, _to, _value)); + // call return false when something wrong require(success); // handle returndata @@ -56,10 +43,10 @@ library SafeERC20 { // Must be a contract addr first! require(_erc20Addr.isContract()); - // call return false when something wrong (bool success, bytes memory returnValue) = // solhint-disable-next-line avoid-low-level-calls _erc20Addr.call(abi.encodeWithSelector(TRANSFERFROM_SELECTOR, _from, _to, _value)); + // call return false when something wrong require(success); // handle returndata @@ -75,13 +62,26 @@ library SafeERC20 { // or when resetting it to zero. require((_value == 0) || (IERC20(_erc20Addr).allowance(msg.sender, _spender) == 0)); - // call return false when something wrong (bool success, bytes memory returnValue) = // solhint-disable-next-line avoid-low-level-calls _erc20Addr.call(abi.encodeWithSelector(APPROVE_SELECTOR, _spender, _value)); + // call return false when something wrong require(success); // handle returndata require(handleReturnData(returnValue)); } + + function handleReturnData(bytes memory returnValue) private pure returns (bool result) { + if (returnValue.length == 0) { + result = true; + } else if (returnValue.length == 32) { + // solhint-disable-next-line no-inline-assembly + assembly { + result := mload(add(returnValue, 32)) + } + } else { + revert(); + } + } } From 081741a990a64eebf44fa05db7badaa4ba0deec2 Mon Sep 17 00:00:00 2001 From: Oren Date: Sun, 13 Jan 2019 09:24:33 +0200 Subject: [PATCH 4/6] optimization --- contracts/libs/SafeERC20.sol | 28 ++++++---------------------- contracts/test/SafeERC20Mock.sol | 5 +---- test/safeerc20.js | 1 - 3 files changed, 7 insertions(+), 27 deletions(-) diff --git a/contracts/libs/SafeERC20.sol b/contracts/libs/SafeERC20.sol index c9999066..3fbbf7c6 100644 --- a/contracts/libs/SafeERC20.sol +++ b/contracts/libs/SafeERC20.sol @@ -33,9 +33,8 @@ library SafeERC20 { _erc20Addr.call(abi.encodeWithSelector(TRANSFER_SELECTOR, _to, _value)); // call return false when something wrong require(success); - - // handle returndata - require(handleReturnData(returnValue)); + //check return value + require(returnValue.length == 0 || (returnValue.length == 32 && (returnValue[31] != 0))); } function safeTransferFrom(address _erc20Addr, address _from, address _to, uint256 _value) internal { @@ -48,9 +47,8 @@ library SafeERC20 { _erc20Addr.call(abi.encodeWithSelector(TRANSFERFROM_SELECTOR, _from, _to, _value)); // call return false when something wrong require(success); - - // handle returndata - require(handleReturnData(returnValue)); + //check return value + require(returnValue.length == 0 || (returnValue.length == 32 && (returnValue[31] != 0))); } function safeApprove(address _erc20Addr, address _spender, uint256 _value) internal { @@ -67,21 +65,7 @@ library SafeERC20 { _erc20Addr.call(abi.encodeWithSelector(APPROVE_SELECTOR, _spender, _value)); // call return false when something wrong require(success); - - // handle returndata - require(handleReturnData(returnValue)); - } - - function handleReturnData(bytes memory returnValue) private pure returns (bool result) { - if (returnValue.length == 0) { - result = true; - } else if (returnValue.length == 32) { - // solhint-disable-next-line no-inline-assembly - assembly { - result := mload(add(returnValue, 32)) - } - } else { - revert(); - } + //check return value + require(returnValue.length == 0 || (returnValue.length == 32 && (returnValue[31] != 0))); } } diff --git a/contracts/test/SafeERC20Mock.sol b/contracts/test/SafeERC20Mock.sol index e26228de..de3f32ad 100644 --- a/contracts/test/SafeERC20Mock.sol +++ b/contracts/test/SafeERC20Mock.sol @@ -15,10 +15,7 @@ contract SafeERC20Mock { } function transfer(address _to, uint256 _value) public returns(bool) { - (, bytes memory returnValue) = - // solhint-disable-next-line avoid-low-level-calls - token.call(abi.encodeWithSignature("transfer(address,uint256)", _to, _value)); - require(returnValue.length > 0); + require(IERC20(token).transfer(_to, _value)); } function transferWithFix(address _to, uint256 _value) public returns(bool) { diff --git a/test/safeerc20.js b/test/safeerc20.js index f20f0e4a..fe6e82cf 100644 --- a/test/safeerc20.js +++ b/test/safeerc20.js @@ -19,7 +19,6 @@ contract('safe erc 20', accounts => { var safeERC20Mock = await SafeERC20Mock.new(badERC20.address); try { await safeERC20Mock.transfer(accounts[1],123); - assert(false, "transfer bad token without the fix revert"); } catch(error) { helpers.assertVMException(error); From aed32c34e28dc02f575b9d208f3a24120d27f9e3 Mon Sep 17 00:00:00 2001 From: Oren Date: Sun, 13 Jan 2019 11:07:21 +0200 Subject: [PATCH 5/6] clean up --- contracts/libs/SafeERC20.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/libs/SafeERC20.sol b/contracts/libs/SafeERC20.sol index 3fbbf7c6..4927510f 100644 --- a/contracts/libs/SafeERC20.sol +++ b/contracts/libs/SafeERC20.sol @@ -1,6 +1,7 @@ /* -badERC20 POC Fix by SECBIT Team +SafeERC20 by daostack. +The code is based on a fix by SECBIT Team. USE WITH CAUTION & NO WARRANTY From 927cb498a97b2c7f6bbe66debbd397c72fce434c Mon Sep 17 00:00:00 2001 From: Oren Date: Sun, 13 Jan 2019 12:13:49 +0200 Subject: [PATCH 6/6] bump version to rc.6 --- package-lock.json | 8 ++++---- package.json | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index f91c53af..75a205f3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@daostack/arc", - "version": "0.0.1-rc.5", + "version": "0.0.1-rc.6", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -199,9 +199,9 @@ } }, "@daostack/infra": { - "version": "0.0.1-rc.6", - "resolved": "https://registry.npmjs.org/@daostack/infra/-/infra-0.0.1-rc.6.tgz", - "integrity": "sha512-SecUB5cLc129SpelfJbsIWeypU92Dr/g/jpInqJ3luZT6mj4+vsD9byBmyXLE+HsrYXTCiOEqcH+TA5oGbi31A==", + "version": "0.0.1-rc.7", + "resolved": "https://registry.npmjs.org/@daostack/infra/-/infra-0.0.1-rc.7.tgz", + "integrity": "sha512-Jvb0bv1kQ3fLwGr8it1faz0vbh2F+5HfM8HQBT2TvV+sOPuPgN0ZY/fPmHELk/E64dygNHjf7It+iydI1kDf5w==", "requires": { "ethereumjs-abi": "^0.6.5", "openzeppelin-solidity": "2.1.1" diff --git a/package.json b/package.json index c5f85d75..ca33f466 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@daostack/arc", - "version": "0.0.1-rc.5", + "version": "0.0.1-rc.6", "description": "A platform for building DAOs", "files": [ "contracts/", @@ -78,7 +78,7 @@ }, "homepage": "https://daostack.io", "dependencies": { - "@daostack/infra": "0.0.1-rc.6", + "@daostack/infra": "0.0.1-rc.7", "ethereumjs-abi": "^0.6.5", "openzeppelin-solidity": "2.1.1", "solhint": "^1.5.0"