From 68fa4a429256046519b526907edc05d209e59e40 Mon Sep 17 00:00:00 2001
From: AndresQuijano <66392159+AndresQuijano@users.noreply.github.com>
Date: Tue, 29 Nov 2022 04:54:40 -0500
Subject: [PATCH] PP-584 (REL-005): Add secure transfer in the Collector (#100)

* feat: add the use of openzeppelin/safeTransfer in the Collector

* feat: add tests for the safeTransfer library

* feat: change the transfer strategy (.call instead of .safeTransfer)

* style: fix the solhint errors

Co-authored-by: Antonio Morrone <antonio@iovlabs.org>
---
 contracts/Collector.sol | 28 ++++++++++++++++--
 test/collector.test.ts  | 64 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/contracts/Collector.sol b/contracts/Collector.sol
index d9057c7f..c1c399c5 100644
--- a/contracts/Collector.sol
+++ b/contracts/Collector.sol
@@ -74,9 +74,19 @@ contract Collector is ICollector{
     noBalanceToShare
     {
         uint256 balance = token.balanceOf(address(this));
+        address tokenAddr = address(token);
 
         if(balance != 0) {
-            token.transfer(remainderAddress, balance);
+            // solhint-disable-next-line avoid-low-level-calls
+            (bool success, bytes memory ret ) = tokenAddr.call{gas: 200000}(abi.encodeWithSelector(
+                hex"a9059cbb",
+                remainderAddress,
+                balance));
+
+            require(
+                success && (ret.length == 0 || abi.decode(ret, (bool))),
+                "Unable to transfer remainder"
+            );
         }
 
         // solhint-disable-next-line
@@ -98,8 +108,20 @@ contract Collector is ICollector{
         uint256 balance = token.balanceOf(address(this));
         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));
+        address tokenAddr = address(token);
+
+        for(uint256 i = 0; i < partners.length; i++){
+            // solhint-disable-next-line avoid-low-level-calls
+            (bool success, bytes memory ret ) = tokenAddr.call(abi.encodeWithSelector(
+                hex"a9059cbb",
+                partners[i].beneficiary,
+                SafeMath.div(SafeMath.mul(balance, partners[i].share), 100)));
+
+            require(
+                success && (ret.length == 0 || abi.decode(ret, (bool))),
+                "Unable to withdraw"
+            );
+        }
     }
 
     function transferOwnership(address _owner)
diff --git a/test/collector.test.ts b/test/collector.test.ts
index 5e9b040d..8e1b35c9 100644
--- a/test/collector.test.ts
+++ b/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.revertedWith('Unable to transfer remainder');
+        });
     });
 
     describe('getBalance', () => {
@@ -377,6 +414,31 @@ describe('Collector', () => {
                 externallyLinkedCollector.withdraw()
             ).to.be.revertedWith('Only owner can call this');
         });
+
+        it('Should fail if the transfer returns false', 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: 1000000 })
+            ).to.be.revertedWith('Unable to withdraw');
+        });
     });
 
     describe('transferOwnership', () => {