PP-584 (REL-005): Add secure transfer in the Collector

contracts/Collector.sol

@@ -5,8 +5,11 @@ pragma experimental ABIEncoderV2;
 import "./interfaces/ICollector.sol";
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@openzeppelin/contracts/math/SafeMath.sol";
+import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
 
 contract Collector is ICollector{
+    using SafeERC20 for IERC20;
+
     address private remainderAddress;
     RevenuePartner[] private partners;
     IERC20 public token;
@@ -76,7 +79,7 @@ contract Collector is ICollector{
         uint256 balance = token.balanceOf(address(this));
 
         if(balance != 0) {
-            token.transfer(remainderAddress, balance);
+            token.safeTransfer(remainderAddress, balance);
         }
 
         // solhint-disable-next-line
@@ -99,7 +102,7 @@ contract Collector is ICollector{
         require(balance >= partners.length, "Not enough balance to split");
 
         for(uint256 i = 0; i < partners.length; i++)
-            token.transfer(partners[i].beneficiary, SafeMath.div(SafeMath.mul(balance, partners[i].share), 100));
+            token.safeTransfer(partners[i].beneficiary, SafeMath.div(SafeMath.mul(balance, partners[i].share), 100));
     }
 
     function transferOwnership(address _owner)

test/collector.test.ts

@@ -1,11 +1,13 @@
 import { expect, use } from 'chai';
 import Collector from '../build/contracts/Collector.json';
 import TestToken from '../build/contracts/TestToken.json';
+import IERC20 from '../build/contracts/IERC20.json';
 import {
     deployContract,
     loadFixture,
     MockProvider,
-    solidity
+    solidity,
+    deployMockContract
 } from 'ethereum-waffle';
 import { Wallet } from '@ethersproject/wallet';
 
@@ -285,6 +287,41 @@ describe('Collector', () => {
                 )
             ).to.be.revertedWith('Only owner can call this');
         });
+
+        it('Should fail if the transfer returns false', async () => {
+            const [
+                owner,
+                oldRemainder,
+                newRemainder,
+                partner1,
+                partner2,
+                partner3,
+                partner4
+            ] = new MockProvider().getWallets();
+
+            const mockERC20 = await deployMockContract(owner, IERC20.abi);
+            await mockERC20.mock.transfer.returns(false);
+            await mockERC20.mock.balanceOf.returns(2);
+
+            const partners = await buildPartners(
+                [partner1, partner2, partner3, partner4],
+                PARTNER_SHARES
+            );
+
+            const collector = await deployContract(owner, Collector, [
+                owner.address,
+                mockERC20.address,
+                partners,
+                oldRemainder.address
+            ]);
+
+            await expect(
+                collector.updateRemainderAddress(newRemainder.address, {
+                    from: owner.address,
+                    gasLimit: 100000
+                })
+            ).to.be.reverted;
+        });
     });
 
     describe('getBalance', () => {
@@ -377,6 +414,31 @@ describe('Collector', () => {
                 externallyLinkedCollector.withdraw()
             ).to.be.revertedWith('Only owner can call this');
         });
+
+        it('Should fail when the transfer fails', async () => {
+            const [owner, remainder, partner1, partner2, partner3, partner4] =
+                new MockProvider().getWallets();
+
+            const mockERC20 = await deployMockContract(owner, IERC20.abi);
+            await mockERC20.mock.transfer.returns(false);
+            await mockERC20.mock.balanceOf.returns(100);
+
+            const partners = await buildPartners(
+                [partner1, partner2, partner3, partner4],
+                PARTNER_SHARES
+            );
+
+            const collector = await deployContract(owner, Collector, [
+                owner.address,
+                mockERC20.address,
+                partners,
+                remainder.address
+            ]);
+
+            await expect(
+                collector.withdraw({ from: owner.address, gasLimit: 100000 })
+            ).to.be.reverted;
+        });
     });
 
     describe('transferOwnership', () => {