From 01b92d1d56175129bab9ec793db980f527f9851c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 9 Oct 2017 19:01:04 -0300 Subject: [PATCH 1/4] fix a problem when revoke is called twice --- contracts/token/TokenVesting.sol | 12 +++++++++--- test/TokenVesting.js | 21 +++++++++++++++------ 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/contracts/token/TokenVesting.sol b/contracts/token/TokenVesting.sol index 3cf576f5b..6d43edff4 100644 --- a/contracts/token/TokenVesting.sol +++ b/contracts/token/TokenVesting.sol @@ -27,6 +27,7 @@ contract TokenVesting is Ownable { bool revocable; mapping (address => uint256) released; + mapping (address => bool) revoked; /** * @dev Creates a vesting contract that vests its balance of any ERC20 token to the @@ -65,17 +66,22 @@ contract TokenVesting is Ownable { } /** - * @notice Allows the owner to revoke the vesting. Tokens already vested remain in the contract. + * @notice Allows the owner to revoke the vesting. Tokens already vested + * remain in the contract, the rest are returned to the owner. * @param token ERC20 token which is being vested */ function revoke(ERC20Basic token) onlyOwner { require(revocable); + require(!revoked[token]); uint256 balance = token.balanceOf(this); uint256 vested = vestedAmount(token); + uint256 vesting = balance - vested; - token.transfer(owner, balance - vested); + revoked[token] = true; + + token.transfer(owner, vesting); Revoked(); } @@ -87,7 +93,7 @@ contract TokenVesting is Ownable { function vestedAmount(ERC20Basic token) constant returns (uint256) { if (now < cliff) { return 0; - } else if (now >= start + duration) { + } else if (now >= start + duration || revoked[token]) { return token.balanceOf(this); } else { uint256 currentBalance = token.balanceOf(this); diff --git a/test/TokenVesting.js b/test/TokenVesting.js index dee1001a7..c391c6577 100644 --- a/test/TokenVesting.js +++ b/test/TokenVesting.js @@ -80,8 +80,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('should return the non-vested tokens when revoked by owner', async function () { - await increaseTimeTo(this.start + this.cliff + duration.weeks(1)); - await this.vesting.release(this.token.address); + await increaseTimeTo(this.start + this.cliff + duration.weeks(12)); const vested = await this.vesting.vestedAmount(this.token.address); const balance = await this.token.balanceOf(this.vesting.address); @@ -93,15 +92,25 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('should keep the vested tokens when revoked by owner', async function () { - await increaseTimeTo(this.start + this.cliff + duration.weeks(1)); - await this.vesting.release(this.token.address); + await increaseTimeTo(this.start + this.cliff + duration.weeks(12)); + + const vestedPre = await this.vesting.vestedAmount(this.token.address); + + await this.vesting.revoke(this.token.address, { from: owner }); + + const vestedPost = await this.vesting.vestedAmount(this.token.address); + + vestedPre.should.bignumber.equal(vestedPost); + }); + + it('should fail to be revoked a second time', async function () { + await increaseTimeTo(this.start + this.cliff + duration.weeks(12)); const vested = await this.vesting.vestedAmount(this.token.address); await this.vesting.revoke(this.token.address, { from: owner }); - const balance = await this.token.balanceOf(this.vesting.address); - balance.should.bignumber.equal(vested); + await this.vesting.revoke(this.token.address, { from: owner }).should.be.rejectedWith(EVMThrow); }); }); From 7d08c4da7fd94898143479ed31dee10d941388a7 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 9 Oct 2017 19:02:57 -0300 Subject: [PATCH 2/4] make TokenVesting variables public --- contracts/token/TokenVesting.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/token/TokenVesting.sol b/contracts/token/TokenVesting.sol index 6d43edff4..2612fd42a 100644 --- a/contracts/token/TokenVesting.sol +++ b/contracts/token/TokenVesting.sol @@ -18,16 +18,16 @@ contract TokenVesting is Ownable { event Revoked(); // beneficiary of tokens after they are released - address beneficiary; + address public beneficiary; - uint256 cliff; - uint256 start; - uint256 duration; + uint256 public cliff; + uint256 public start; + uint256 public duration; - bool revocable; + bool public revocable; - mapping (address => uint256) released; - mapping (address => bool) revoked; + mapping (address => uint256) public released; + mapping (address => bool) public revoked; /** * @dev Creates a vesting contract that vests its balance of any ERC20 token to the From a184013d1e928f39e0bbf88cc98019c62f833a8e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 9 Oct 2017 19:08:32 -0300 Subject: [PATCH 3/4] explicitly mark functions public --- contracts/token/TokenVesting.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/token/TokenVesting.sol b/contracts/token/TokenVesting.sol index 2612fd42a..c8e8c9214 100644 --- a/contracts/token/TokenVesting.sol +++ b/contracts/token/TokenVesting.sol @@ -53,7 +53,7 @@ contract TokenVesting is Ownable { * @notice Transfers vested tokens to beneficiary. * @param token ERC20 token which is being vested */ - function release(ERC20Basic token) { + function release(ERC20Basic token) public { uint256 vested = vestedAmount(token); require(vested > 0); @@ -70,7 +70,7 @@ contract TokenVesting is Ownable { * remain in the contract, the rest are returned to the owner. * @param token ERC20 token which is being vested */ - function revoke(ERC20Basic token) onlyOwner { + function revoke(ERC20Basic token) public onlyOwner { require(revocable); require(!revoked[token]); @@ -90,7 +90,7 @@ contract TokenVesting is Ownable { * @dev Calculates the amount that has already vested but hasn't been released yet. * @param token ERC20 token which is being vested */ - function vestedAmount(ERC20Basic token) constant returns (uint256) { + function vestedAmount(ERC20Basic token) public constant returns (uint256) { if (now < cliff) { return 0; } else if (now >= start + duration || revoked[token]) { From d5e0714faf4b139aac319bab6ad3a10cd0918640 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 11 Oct 2017 16:31:31 -0300 Subject: [PATCH 4/4] use safeTransfer --- contracts/token/TokenVesting.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/token/TokenVesting.sol b/contracts/token/TokenVesting.sol index c8e8c9214..2d8eabb6a 100644 --- a/contracts/token/TokenVesting.sol +++ b/contracts/token/TokenVesting.sol @@ -1,6 +1,7 @@ pragma solidity ^0.4.11; import './ERC20Basic.sol'; +import './SafeERC20.sol'; import '../ownership/Ownable.sol'; import '../math/Math.sol'; import '../math/SafeMath.sol'; @@ -13,6 +14,7 @@ import '../math/SafeMath.sol'; */ contract TokenVesting is Ownable { using SafeMath for uint256; + using SafeERC20 for ERC20Basic; event Released(uint256 amount); event Revoked(); @@ -58,7 +60,7 @@ contract TokenVesting is Ownable { require(vested > 0); - token.transfer(beneficiary, vested); + token.safeTransfer(beneficiary, vested); released[token] = released[token].add(vested); @@ -81,7 +83,7 @@ contract TokenVesting is Ownable { revoked[token] = true; - token.transfer(owner, vesting); + token.safeTransfer(owner, vesting); Revoked(); }