From b76ff570e2f9804e0df350afa311ebc298ec9485 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Quijano?= Date: Tue, 22 Nov 2022 17:34:11 -0500 Subject: [PATCH 1/4] feat: add the use of openzeppelin/safeTransfer in the Collector --- contracts/Collector.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index d9057c7f..ada7ccda 100644 --- a/contracts/Collector.sol +++ b/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) From 70d6f69b52fd0ffc2e9acbef2430e1bf5c642c90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Quijano?= Date: Wed, 23 Nov 2022 17:49:54 -0500 Subject: [PATCH 2/4] feat: add tests for the safeTransfer library --- test/collector.test.ts | 64 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/test/collector.test.ts b/test/collector.test.ts index 5e9b040d..f0ea9122 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.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', () => { From 63a658e8e9941c03505aa7da33bd74ce837395e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Quijano?= Date: Mon, 28 Nov 2022 10:32:24 -0500 Subject: [PATCH 3/4] feat: change the transfer strategy (.call instead of .safeTransfer) --- contracts/Collector.sol | 29 +++++++++++++++++++++++------ test/collector.test.ts | 8 ++++---- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index ada7ccda..a1e6f3f1 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -5,11 +5,8 @@ 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; @@ -77,9 +74,18 @@ contract Collector is ICollector{ noBalanceToShare { uint256 balance = token.balanceOf(address(this)); + address tokenAddr = address(token); if(balance != 0) { - token.safeTransfer(remainderAddress, balance); + (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 @@ -101,8 +107,19 @@ 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.safeTransfer(partners[i].beneficiary, SafeMath.div(SafeMath.mul(balance, partners[i].share), 100)); + address tokenAddr = address(token); + + for(uint256 i = 0; i < partners.length; i++){ + (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 f0ea9122..8e1b35c9 100644 --- a/test/collector.test.ts +++ b/test/collector.test.ts @@ -320,7 +320,7 @@ describe('Collector', () => { from: owner.address, gasLimit: 100000 }) - ).to.be.reverted; + ).to.be.revertedWith('Unable to transfer remainder'); }); }); @@ -415,7 +415,7 @@ describe('Collector', () => { ).to.be.revertedWith('Only owner can call this'); }); - it('Should fail when the transfer fails', async () => { + it('Should fail if the transfer returns false', async () => { const [owner, remainder, partner1, partner2, partner3, partner4] = new MockProvider().getWallets(); @@ -436,8 +436,8 @@ describe('Collector', () => { ]); await expect( - collector.withdraw({ from: owner.address, gasLimit: 100000 }) - ).to.be.reverted; + collector.withdraw({ from: owner.address, gasLimit: 1000000 }) + ).to.be.revertedWith('Unable to withdraw'); }); }); From 5db20511ac16baa0ae20fb8bb77ae1b62c8be27b Mon Sep 17 00:00:00 2001 From: Antonio Morrone Date: Mon, 28 Nov 2022 17:36:54 +0100 Subject: [PATCH 4/4] style: fix the solhint errors --- contracts/Collector.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index a1e6f3f1..c1c399c5 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -77,6 +77,7 @@ contract Collector is ICollector{ address tokenAddr = address(token); if(balance != 0) { + // solhint-disable-next-line avoid-low-level-calls (bool success, bytes memory ret ) = tokenAddr.call{gas: 200000}(abi.encodeWithSelector( hex"a9059cbb", remainderAddress, @@ -110,6 +111,7 @@ contract Collector is ICollector{ 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,