Increase testing coverage (#1195)

* Added non-target bounty test

* Increased ERC721 testing coverage.

* Addressed review comments.

* fix linter error

* Fixed linter error

* Removed unnecessary bouncer require

* Improved Crowdsale tests.

* Added missing SuperUser test.

* Improved payment tests.

* Improved token tests.

* Fixed ERC721 test.

* Reviewed phrasing.

contracts/access/SignatureBouncer.sol

@@ -82,7 +82,6 @@ contract SignatureBouncer is Ownable, RBAC {
     public
     onlyOwner
   {
-    require(_bouncer != address(0));
     removeRole(_bouncer, ROLE_BOUNCER);
   }
 

contracts/token/ERC721/ERC721BasicToken.sol

@@ -131,7 +131,6 @@ contract ERC721BasicToken is SupportsInterfaceWithLookup, ERC721Basic {
     public
   {
     require(isApprovedOrOwner(msg.sender, _tokenId));
-    require(_from != address(0));
     require(_to != address(0));
 
     clearApproval(_from, _tokenId);

test/Bounty.test.js

@@ -17,7 +17,7 @@ const sendReward = async (from, to, value) => ethSendTransaction({
 
 const reward = new web3.BigNumber(web3.toWei(1, 'ether'));
 
-contract('Bounty', function ([_, owner, researcher]) {
+contract('Bounty', function ([_, owner, researcher, nonTarget]) {
   context('against secure contract', function () {
     beforeEach(async function () {
       this.bounty = await SecureTargetBounty.new({ from: owner });
@@ -82,6 +82,12 @@ contract('Bounty', function ([_, owner, researcher]) {
       researcherCurrBalance.sub(researcherPrevBalance).should.be.bignumber.equal(reward.sub(gasCost));
     });
 
+    it('cannot claim reward from non-target', async function () {
+      await assertRevert(
+        this.bounty.claim(nonTarget, { from: researcher })
+      );
+    });
+
     context('reward claimed', function () {
       beforeEach(async function () {
         await this.bounty.claim(this.targetAddress, { from: researcher });

test/crowdsale/Crowdsale.test.js

@@ -1,3 +1,4 @@
+const { assertRevert } = require('../helpers/assertRevert');
 const { ether } = require('../helpers/ether');
 const { ethGetBalance } = require('../helpers/web3');
 
@@ -15,18 +16,67 @@ contract('Crowdsale', function ([_, investor, wallet, purchaser]) {
   const value = ether(42);
   const tokenSupply = new BigNumber('1e22');
   const expectedTokenAmount = rate.mul(value);
+  const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
 
+  it('requires a non-null token', async function () {
+    await assertRevert(
+      Crowdsale.new(rate, wallet, ZERO_ADDRESS)
+    );
+  });
+
+  context('with token', async function () {
     beforeEach(async function () {
       this.token = await SimpleToken.new();
+    });
+
+    it('requires a non-zero rate', async function () {
+      await assertRevert(
+        Crowdsale.new(0, wallet, this.token.address)
+      );
+    });
+
+    it('requires a non-null wallet', async function () {
+      await assertRevert(
+        Crowdsale.new(rate, ZERO_ADDRESS, this.token.address)
+      );
+    });
+
+    context('once deployed', async function () {
+      beforeEach(async function () {
         this.crowdsale = await Crowdsale.new(rate, wallet, this.token.address);
         await this.token.transfer(this.crowdsale.address, tokenSupply);
       });
 
       describe('accepting payments', function () {
+        describe('bare payments', function () {
+          it('should accept payments', async function () {
+            await this.crowdsale.send(value, { from: purchaser });
+          });
+
+          it('reverts on zero-valued payments', async function () {
+            await assertRevert(
+              this.crowdsale.send(0, { from: purchaser })
+            );
+          });
+        });
+
+        describe('buyTokens', function () {
           it('should accept payments', async function () {
-      await this.crowdsale.send(value);
             await this.crowdsale.buyTokens(investor, { value: value, from: purchaser });
           });
+
+          it('reverts on zero-valued payments', async function () {
+            await assertRevert(
+              this.crowdsale.buyTokens(investor, { value: 0, from: purchaser })
+            );
+          });
+
+          it('requires a non-null beneficiary', async function () {
+            await assertRevert(
+              this.crowdsale.buyTokens(ZERO_ADDRESS, { value: value, from: purchaser })
+            );
+          });
+        });
       });
 
       describe('high-level purchase', function () {
@@ -78,4 +128,6 @@ contract('Crowdsale', function ([_, investor, wallet, purchaser]) {
           post.minus(pre).should.be.bignumber.equal(value);
         });
       });
+    });
+  });
 });

test/ownership/Superuser.test.js

@@ -7,6 +7,8 @@ require('chai')
   .should();
 
 contract('Superuser', function ([_, firstOwner, newSuperuser, newOwner, anyone]) {
+  const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
+
   beforeEach(async function () {
     this.superuser = await Superuser.new({ from: firstOwner });
   });
@@ -27,6 +29,12 @@ contract('Superuser', function ([_, firstOwner, newSuperuser, newOwner, anyone])
       newSuperuserIsSuperuser.should.be.equal(true);
     });
 
+    it('should prevent changing to a null superuser', async function () {
+      await expectThrow(
+        this.superuser.transferSuperuser(ZERO_ADDRESS, { from: firstOwner })
+      );
+    });
+
     it('should change owner after the superuser transfers the ownership', async function () {
       await this.superuser.transferSuperuser(newSuperuser, { from: firstOwner });
 

test/payment/RefundEscrow.test.js

@@ -14,7 +14,15 @@ const RefundEscrow = artifacts.require('RefundEscrow');
 contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2]) {
   const amount = web3.toWei(54.0, 'ether');
   const refundees = [refundee1, refundee2];
+  const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
 
+  it('requires a non-null beneficiary', async function () {
+    await expectThrow(
+      RefundEscrow.new(ZERO_ADDRESS, { from: owner })
+    );
+  });
+
+  context('once deployed', function () {
     beforeEach(async function () {
       this.escrow = await RefundEscrow.new(beneficiary, { from: owner });
     });
@@ -68,6 +76,14 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2]
 
         beneficiaryFinalBalance.sub(beneficiaryInitialBalance).should.be.bignumber.equal(amount * refundees.length);
       });
+
+      it('prevents entering the refund state', async function () {
+        await expectThrow(this.escrow.enableRefunds({ from: owner }), EVMRevert);
+      });
+
+      it('prevents re-entering the closed state', async function () {
+        await expectThrow(this.escrow.close({ from: owner }), EVMRevert);
+      });
     });
 
     it('only owner can enter refund state', async function () {
@@ -102,5 +118,14 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2]
       it('does not allow beneficiary withdrawal', async function () {
         await expectThrow(this.escrow.beneficiaryWithdraw(), EVMRevert);
       });
+
+      it('prevents entering the closed state', async function () {
+        await expectThrow(this.escrow.close({ from: owner }), EVMRevert);
+      });
+
+      it('prevents re-entering the refund state', async function () {
+        await expectThrow(this.escrow.enableRefunds({ from: owner }), EVMRevert);
+      });
+    });
   });
 });

test/payment/SplitPayment.test.js

@@ -12,6 +12,7 @@ const SplitPayment = artifacts.require('SplitPayment');
 
 contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, payer1]) {
   const amount = web3.toWei(1.0, 'ether');
+  const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
 
   it('cannot be created with no payees', async function () {
     await expectThrow(SplitPayment.new([], []), EVMRevert);
@@ -25,6 +26,18 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1,
     await expectThrow(SplitPayment.new([payee1, payee2], [20, 30, 40]), EVMRevert);
   });
 
+  it('requires non-null payees', async function () {
+    await expectThrow(SplitPayment.new([payee1, ZERO_ADDRESS], [20, 30]), EVMRevert);
+  });
+
+  it('requires non-zero shares', async function () {
+    await expectThrow(SplitPayment.new([payee1, payee2], [20, 0]), EVMRevert);
+  });
+
+  it('rejects repeated payees', async function () {
+    await expectThrow(SplitPayment.new([payee1, payee1], [20, 0]), EVMRevert);
+  });
+
   context('once deployed', function () {
     beforeEach(async function () {
       this.payees = [payee1, payee2, payee3];

test/token/ERC20/CappedToken.test.js

@@ -1,3 +1,4 @@
+const { assertRevert } = require('../../helpers/assertRevert');
 const { ether } = require('../../helpers/ether');
 const { shouldBehaveLikeMintableToken } = require('./MintableToken.behavior');
 const { shouldBehaveLikeCappedToken } = require('./CappedToken.behavior');
@@ -7,10 +8,18 @@ const CappedToken = artifacts.require('CappedToken');
 contract('Capped', function ([_, owner, ...otherAccounts]) {
   const cap = ether(1000);
 
+  it('requires a non-zero cap', async function () {
+    await assertRevert(
+      CappedToken.new(0, { from: owner })
+    );
+  });
+
+  context('once deployed', async function () {
     beforeEach(async function () {
       this.token = await CappedToken.new(cap, { from: owner });
     });
 
     shouldBehaveLikeCappedToken(owner, otherAccounts, cap);
     shouldBehaveLikeMintableToken(owner, owner, otherAccounts);
+  });
 });

test/token/ERC20/TokenTimelock.test.js

@@ -14,8 +14,20 @@ const TokenTimelock = artifacts.require('TokenTimelock');
 contract('TokenTimelock', function ([_, owner, beneficiary]) {
   const amount = new BigNumber(100);
 
+  context('with token', function () {
     beforeEach(async function () {
       this.token = await MintableToken.new({ from: owner });
+    });
+
+    it('rejects a release time in the past', async function () {
+      const pastReleaseTime = (await latestTime()) - duration.years(1);
+      await expectThrow(
+        TokenTimelock.new(this.token.address, beneficiary, pastReleaseTime)
+      );
+    });
+
+    context('once deployed', function () {
+      beforeEach(async function () {
         this.releaseTime = (await latestTime()) + duration.years(1);
         this.timelock = await TokenTimelock.new(this.token.address, beneficiary, this.releaseTime);
         await this.token.mint(this.timelock.address, amount, { from: owner });
@@ -51,4 +63,6 @@ contract('TokenTimelock', function ([_, owner, beneficiary]) {
         const balance = await this.token.balanceOf(beneficiary);
         balance.should.be.bignumber.equal(amount);
       });
+    });
+  });
 });

test/token/ERC20/TokenVesting.test.js

@@ -15,16 +15,36 @@ const TokenVesting = artifacts.require('TokenVesting');
 
 contract('TokenVesting', function ([_, owner, beneficiary]) {
   const amount = new BigNumber(1000);
+  const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
 
   beforeEach(async function () {
-    this.token = await MintableToken.new({ from: owner });
-
     this.start = (await latestTime()) + duration.minutes(1); // +1 minute so it starts after contract instantiation
     this.cliff = duration.years(1);
     this.duration = duration.years(2);
+  });
 
+  it('rejects a duration shorter than the cliff', async function () {
+    const cliff = this.duration;
+    const duration = this.cliff;
+
+    cliff.should.be.gt(duration);
+
+    await expectThrow(
+      TokenVesting.new(beneficiary, this.start, cliff, duration, true, { from: owner })
+    );
+  });
+
+  it('requires a valid beneficiary', async function () {
+    await expectThrow(
+      TokenVesting.new(ZERO_ADDRESS, this.start, this.cliff, this.duration, true, { from: owner })
+    );
+  });
+
+  context('once deployed', function () {
+    beforeEach(async function () {
       this.vesting = await TokenVesting.new(beneficiary, this.start, this.cliff, this.duration, true, { from: owner });
 
+      this.token = await MintableToken.new({ from: owner });
       await this.token.mint(this.vesting.address, amount, { from: owner });
     });
 
@@ -79,7 +99,10 @@ contract('TokenVesting', function ([_, owner, beneficiary]) {
     });
 
     it('should fail to be revoked by owner if revocable not set', async function () {
-    const vesting = await TokenVesting.new(beneficiary, this.start, this.cliff, this.duration, false, { from: owner });
+      const vesting = await TokenVesting.new(
+        beneficiary, this.start, this.cliff, this.duration, false, { from: owner }
+      );
+
       await expectThrow(
         vesting.revoke(this.token.address, { from: owner }),
         EVMRevert,
@@ -121,4 +144,5 @@ contract('TokenVesting', function ([_, owner, beneficiary]) {
         EVMRevert,
       );
     });
+  });
 });

test/token/ERC721/ERC721Token.test.js

@@ -16,7 +16,9 @@ contract('ERC721Token', function (accounts) {
   const symbol = 'NFT';
   const firstTokenId = 100;
   const secondTokenId = 200;
+  const nonExistentTokenId = 999;
   const creator = accounts[0];
+  const anyone = accounts[9];
 
   beforeEach(async function () {
     this.token = await ERC721Token.new(name, symbol, { from: creator });
@@ -77,6 +79,13 @@ contract('ERC721Token', function (accounts) {
     });
 
     describe('removeTokenFrom', function () {
+      it('reverts if the correct owner is not passed', async function () {
+        await assertRevert(
+          this.token._removeTokenFrom(anyone, firstTokenId, { from: creator })
+        );
+      });
+
+      context('once removed', function () {
         beforeEach(async function () {
           await this.token._removeTokenFrom(creator, firstTokenId, { from: creator });
         });
@@ -100,6 +109,7 @@ contract('ERC721Token', function (accounts) {
           total.toNumber().should.be.equal(2);
         });
       });
+    });
 
     describe('metadata', function () {
       const sampleUri = 'mock://mytoken';
@@ -120,6 +130,10 @@ contract('ERC721Token', function (accounts) {
         uri.should.be.equal(sampleUri);
       });
 
+      it('reverts when setting metadata for non existent token id', async function () {
+        await assertRevert(this.token.setTokenURI(nonExistentTokenId, sampleUri));
+      });
+
       it('can burn token with metadata', async function () {
         await this.token.setTokenURI(firstTokenId, sampleUri);
         await this.token.burn(firstTokenId);
@@ -132,8 +146,8 @@ contract('ERC721Token', function (accounts) {
         uri.should.be.equal('');
       });
 
-      it('reverts when querying metadata for non existant token id', async function () {
+      it('reverts when querying metadata for non existent token id', async function () {
-        await assertRevert(this.token.tokenURI(500));
+        await assertRevert(this.token.tokenURI(nonExistentTokenId));
       });
     });