From 1b585434944f10815f0de0ddbea124fb118bad85 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 09:34:59 +0200 Subject: [PATCH 01/21] update voucher hub whit check on transfer --- contracts/VoucherHub.sol | 10 ++++++++-- test/VoucherHub.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 6ce997ce..3f1a732e 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -172,7 +172,10 @@ contract VoucherHub is // The proxy contract does a delegatecall to its implementation. // Re-Entrancy safe because the target contract is controlled. Voucher(voucherAddress).initialize(owner, address(this), expiration, voucherType); - IERC20($._iexecPoco).transfer(voucherAddress, value); // SRLC + if (!IERC20($._iexecPoco).transfer(voucherAddress, value)) { + // SRLC + revert("VoucherHub: SRLC transfer to voucher failed"); + } _mint(voucherAddress, value); // VCHR $._isVoucher[voucherAddress] = true; emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); @@ -188,7 +191,10 @@ contract VoucherHub is VoucherHubStorage storage $ = _getVoucherHubStorage(); require($._isVoucher[voucher], "VoucherHub: unknown voucher"); _mint(voucher, value); // VCHR - IERC20($._iexecPoco).transfer(voucher, value); // SRLC + if (!IERC20($._iexecPoco).transfer(voucher, value)) { + // SRLC + revert("VoucherHub: SRLC transfer to voucher failed"); + } uint256 expiration = block.timestamp + $.voucherTypes[Voucher(voucher).getType()].duration; Voucher(voucher).setExpiration(expiration); emit VoucherToppedUp(voucher, expiration, value); diff --git a/test/VoucherHub.test.ts b/test/VoucherHub.test.ts index 6eead67b..1ddf9d14 100644 --- a/test/VoucherHub.test.ts +++ b/test/VoucherHub.test.ts @@ -668,6 +668,22 @@ describe('VoucherHub', function () { ), ).to.be.revertedWith('VoucherHub: type index out of bounds'); }); + it('Should not create voucher when SLRC transfer fails', async function () { + const { voucherOwner1 } = await loadFixture(deployFixture); + await voucherHubWithAssetEligibilityManagerSigner.createVoucherType( + description, + duration, + ); + await iexecPocoInstance.willRevertOnTransfer().then((tx) => tx.wait()); + // Create voucher. + await expect( + voucherHubWithVoucherManagerSigner.createVoucher( + voucherOwner1, + voucherType, + voucherValue, + ), + ).to.be.revertedWith('VoucherHub: SRLC transfer to voucher failed'); + }); }); describe('Top up voucher', function () { @@ -737,6 +753,14 @@ describe('VoucherHub', function () { ), ).to.revertedWith('VoucherHub: unknown voucher'); }); + it('Should not top up when SLRC transfer fails', async function () { + const topUpValue = 123n; // arbitrary value + await iexecPocoInstance.willRevertOnTransfer().then((tx) => tx.wait()); + // Create voucher. + await expect( + voucherHubWithVoucherManagerSigner.topUpVoucher(voucherAddress, topUpValue), + ).to.be.revertedWith('VoucherHub: SRLC transfer to voucher failed'); + }); }); describe('Debit voucher', function () { From 5efcfc4ea5282a18900fe0d0e9283b41ceb5d295 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 09:35:16 +0200 Subject: [PATCH 02/21] add revert on transfer and transfer From --- contracts/mocks/IexecPocoMock.sol | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/contracts/mocks/IexecPocoMock.sol b/contracts/mocks/IexecPocoMock.sol index 37a95e77..53039050 100644 --- a/contracts/mocks/IexecPocoMock.sol +++ b/contracts/mocks/IexecPocoMock.sol @@ -18,6 +18,8 @@ contract IexecPocoMock is ERC20 { bool public shouldRevertOnSponsorMatchOrdersBoost = false; bool public shouldRevertOnClaim = false; bool public shouldReturnTaskWithFailedStatus = false; + bool public shouldRevertOnTransfer = false; + bool public shouldRevertOnTransferFrom = false; bytes32 public mockDealId = keccak256("deal"); uint256 public mockTaskIndex = 0; @@ -92,6 +94,14 @@ contract IexecPocoMock is ERC20 { shouldRevertOnSponsorMatchOrdersBoost = true; } + function willRevertOnTransfer() external { + shouldRevertOnTransfer = true; + } + + function willRevertOnTransferFrom() external { + shouldRevertOnTransferFrom = true; + } + /** * Claim */ @@ -155,4 +165,25 @@ contract IexecPocoMock is ERC20 { requestOrder.volume ); } + + /** + * Override transfer and transferFrom to mock revert case + */ + function transfer(address recipient, uint256 amount) public override returns (bool) { + if (shouldRevertOnTransfer) { + return !shouldRevertOnTransfer; + } + return super.transfer(recipient, amount); + } + + function transferFrom( + address sender, + address recipient, + uint256 amount + ) public override returns (bool) { + if (shouldRevertOnTransferFrom) { + return !shouldRevertOnTransferFrom; + } + return super.transferFrom(sender, recipient, amount); + } } From 147329d13bad078a545cb0d130264fa93f6a21ab Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 10:42:55 +0200 Subject: [PATCH 03/21] check erc transfer --- contracts/beacon/Voucher.sol | 19 +++++++++++----- test/beacon/Voucher.test.ts | 44 ++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index 944809f4..eda87c6c 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -373,7 +373,9 @@ contract Voucher is Initializable, IVoucher { // If the deal was not sponsored or partially sponsored // by the voucher then send the non-sponsored part back // to the requester. - IERC20(iexecPoco).transfer(requester, taskPrice - taskSponsoredAmount); + if (!IERC20(iexecPoco).transfer(requester, taskPrice - taskSponsoredAmount)) { + revert("Voucher: transfer to requester failed"); + } } } } @@ -432,11 +434,16 @@ contract Voucher is Initializable, IVoucher { if (sponsoredAmount != dealPrice) { // Transfer non-sponsored amount from the iExec account of the // requester to the iExec account of the voucher - IERC20(iexecPoco).transferFrom( - requestOrder.requester, - address(this), - dealPrice - sponsoredAmount - ); + if ( + !IERC20(iexecPoco).transferFrom( + requestOrder.requester, + address(this), + dealPrice - sponsoredAmount + ) + ) { + // SRLC + revert("Voucher: Transfer of non-sponsored amount failed"); + } } } } diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index 73cf1f68..df9bf025 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -644,6 +644,28 @@ describe('Voucher', function () { ), ).to.be.revertedWith('IexecPocoMock: Failed to sponsorMatchOrdersBoost'); }); + + it('Should not match orders boost when SRLC transfer fails', async () => { + const noSponsoredValue = appPrice * volume; + await addEligibleAssets([dataset, workerpool]); + await iexecPocoInstance + .transfer(requester, noSponsoredValue) + .then((tx) => tx.wait()); + + await iexecPocoInstance + .connect(requester) + .approve(await voucherAsOwner.getAddress(), noSponsoredValue) + .then((tx) => tx.wait()); + await iexecPocoInstance.willRevertOnTransferFrom().then((tx) => tx.wait()); + await expect( + voucherAsOwner.matchOrdersBoost( + appOrder, + datasetOrder, + workerpoolOrder, + requestOrder, + ), + ).to.be.revertedWith('Voucher: Transfer of non-sponsored amount failed'); + }); }); }); @@ -1049,6 +1071,28 @@ describe('Voucher', function () { ); }); }); + describe('Should not claim task when SLRC transfer fails', async () => { + it('Classic', async () => await runTest(voucherMatchOrders, claim)); + it('Boost', async () => await runTest(voucherMatchOrdersBoost, claimBoost)); + + async function runTest(matchOrdersBoostOrClassic: any, claimBoostOrClassic: any) { + await addEligibleAssets([app, dataset]); // workerpool not eligible. + const dealNonSponsoredAmount = workerpoolPrice * volume; + // Deposit non-sponsored amount for requester and approve voucher. + await iexecPocoInstance + .transfer(requester, dealNonSponsoredAmount) + .then((tx) => tx.wait()); + await iexecPocoInstance + .connect(requester) + .approve(voucherAddress, dealNonSponsoredAmount) + .then((tx) => tx.wait()); + await matchOrdersBoostOrClassic(); + await iexecPocoInstance.willRevertOnTransfer().then((tx) => tx.wait()); + await expect(claimBoostOrClassic()).to.be.revertedWith( + 'Voucher: transfer to requester failed', + ); + } + }); }); async function addEligibleAssets(assets: string[]) { From 1b2ae898d5cd4c7384bd55eee623c0443ae292a3 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 11:31:32 +0200 Subject: [PATCH 04/21] add slither-disable for arbitrary-send-erc20 on voucher SC --- contracts/beacon/Voucher.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index eda87c6c..44727a5b 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -434,6 +434,7 @@ contract Voucher is Initializable, IVoucher { if (sponsoredAmount != dealPrice) { // Transfer non-sponsored amount from the iExec account of the // requester to the iExec account of the voucher + //slither-disable-start arbitrary-send-erc20 if ( !IERC20(iexecPoco).transferFrom( requestOrder.requester, @@ -444,6 +445,7 @@ contract Voucher is Initializable, IVoucher { // SRLC revert("Voucher: Transfer of non-sponsored amount failed"); } + //slither-disable-end arbitrary-send-erc20 } } } From 1d4a266cedd65078e05831343083612793bf1ad1 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 12:01:57 +0200 Subject: [PATCH 05/21] add slither config --- slither.config.json | 1 + 1 file changed, 1 insertion(+) diff --git a/slither.config.json b/slither.config.json index 5aecaffd..47f9bd19 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,5 +1,6 @@ { "solc_remaps": "@=node_modules/@", "filter_paths": "(mocks/|node_modules/)", + "detectors_to_exclude": "pragma,solc-version,assembly,too-many-digits", "solc_args": "--optimize --via-ir" } From 4f35a750431bcf3e947b19ef784024fa1087c5ea Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 12:06:49 +0200 Subject: [PATCH 06/21] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 477ba8ba..613d2758 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## vNEXT +- Add slither suggestions. (#26) - Add slither github action. (#24) - Top up voucher. (#23) - Claim task part 2 - Add voucher tests. (#21) From 248e7f29099f9720d68f414ecdd4d87d98a461c7 Mon Sep 17 00:00:00 2001 From: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com> Date: Mon, 10 Jun 2024 15:11:07 +0200 Subject: [PATCH 07/21] Update contracts/VoucherHub.sol Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com> --- contracts/VoucherHub.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 3f1a732e..43480657 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -172,8 +172,8 @@ contract VoucherHub is // The proxy contract does a delegatecall to its implementation. // Re-Entrancy safe because the target contract is controlled. Voucher(voucherAddress).initialize(owner, address(this), expiration, voucherType); + // SRLC if (!IERC20($._iexecPoco).transfer(voucherAddress, value)) { - // SRLC revert("VoucherHub: SRLC transfer to voucher failed"); } _mint(voucherAddress, value); // VCHR From e8214eb74b1aabba441a5573caf6e896828009df Mon Sep 17 00:00:00 2001 From: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com> Date: Mon, 10 Jun 2024 15:11:35 +0200 Subject: [PATCH 08/21] Update contracts/mocks/IexecPocoMock.sol Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com> --- contracts/mocks/IexecPocoMock.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/mocks/IexecPocoMock.sol b/contracts/mocks/IexecPocoMock.sol index 53039050..13b5d2a2 100644 --- a/contracts/mocks/IexecPocoMock.sol +++ b/contracts/mocks/IexecPocoMock.sol @@ -170,7 +170,7 @@ contract IexecPocoMock is ERC20 { * Override transfer and transferFrom to mock revert case */ function transfer(address recipient, uint256 amount) public override returns (bool) { - if (shouldRevertOnTransfer) { + if (shouldFailOnTransfer) { return !shouldRevertOnTransfer; } return super.transfer(recipient, amount); From f605ed16f9b900a8f0d1e9e69609adcb7dd99b99 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 15:28:58 +0200 Subject: [PATCH 09/21] update mock --- contracts/mocks/IexecPocoMock.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/mocks/IexecPocoMock.sol b/contracts/mocks/IexecPocoMock.sol index 13b5d2a2..8c07f580 100644 --- a/contracts/mocks/IexecPocoMock.sol +++ b/contracts/mocks/IexecPocoMock.sol @@ -18,8 +18,8 @@ contract IexecPocoMock is ERC20 { bool public shouldRevertOnSponsorMatchOrdersBoost = false; bool public shouldRevertOnClaim = false; bool public shouldReturnTaskWithFailedStatus = false; - bool public shouldRevertOnTransfer = false; - bool public shouldRevertOnTransferFrom = false; + bool public shouldFailOnTransfer = false; + bool public shouldFailOnTransferFrom = false; bytes32 public mockDealId = keccak256("deal"); uint256 public mockTaskIndex = 0; @@ -95,11 +95,11 @@ contract IexecPocoMock is ERC20 { } function willRevertOnTransfer() external { - shouldRevertOnTransfer = true; + shouldFailOnTransfer = true; } function willRevertOnTransferFrom() external { - shouldRevertOnTransferFrom = true; + shouldFailOnTransferFrom = true; } /** @@ -171,7 +171,7 @@ contract IexecPocoMock is ERC20 { */ function transfer(address recipient, uint256 amount) public override returns (bool) { if (shouldFailOnTransfer) { - return !shouldRevertOnTransfer; + return false; } return super.transfer(recipient, amount); } @@ -181,8 +181,8 @@ contract IexecPocoMock is ERC20 { address recipient, uint256 amount ) public override returns (bool) { - if (shouldRevertOnTransferFrom) { - return !shouldRevertOnTransferFrom; + if (shouldFailOnTransferFrom) { + return false; } return super.transferFrom(sender, recipient, amount); } From 0e654fef1d66ec22930567f5c622fa6029fb0626 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 15:29:15 +0200 Subject: [PATCH 10/21] add note on slither disable --- contracts/beacon/Voucher.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index 44727a5b..5bb60ae9 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -435,6 +435,8 @@ contract Voucher is Initializable, IVoucher { // Transfer non-sponsored amount from the iExec account of the // requester to the iExec account of the voucher //slither-disable-start arbitrary-send-erc20 + // Note: We can disable this check since request order is signed by requester so he agrees to pay for it + // & caller is only authorized. if ( !IERC20(iexecPoco).transferFrom( requestOrder.requester, From 5a99207139e157f41c5b631bcf91e4856098b825 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 16:49:54 +0200 Subject: [PATCH 11/21] add slither too many digits in contract --- contracts/VoucherHub.sol | 3 +++ slither.config.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 43480657..2ec16c47 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -86,12 +86,15 @@ contract VoucherHub is VoucherHubStorage storage $ = _getVoucherHubStorage(); $._iexecPoco = iexecPoco; $._voucherBeacon = voucherBeacon; + //slither-disable-start too-many-digits + // See : https://github.com/crytic/slither/issues/1223 $._voucherCreationCodeHash = keccak256( abi.encodePacked( type(VoucherProxy).creationCode, // bytecode abi.encode($._voucherBeacon) // constructor args ) ); + //slither-disable-end too-many-digits } function createVoucherType( diff --git a/slither.config.json b/slither.config.json index 47f9bd19..d47b4692 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,6 +1,6 @@ { "solc_remaps": "@=node_modules/@", "filter_paths": "(mocks/|node_modules/)", - "detectors_to_exclude": "pragma,solc-version,assembly,too-many-digits", + "detectors_to_exclude": "pragma,solc-version,assembly", "solc_args": "--optimize --via-ir" } From c57e4ab2cca55b7bba13eca7d5c25a2b9b381ac7 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 16:56:03 +0200 Subject: [PATCH 12/21] remove unused imports --- contracts/VoucherHub.sol | 11 ++++++++++- contracts/beacon/Voucher.sol | 1 - 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 2ec16c47..92a2d1db 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -7,7 +7,6 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol"; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import {Voucher} from "./beacon/Voucher.sol"; @@ -341,4 +340,14 @@ contract VoucherHub is function _getCreate2Salt(address account) private pure returns (bytes32) { return bytes32(uint256(uint160(account))); } + + function _transfertFundsToVoucher( + address iexecPoco, + address voucherAddress, + uint256 value + ) private { + if (!IERC20(iexecPoco).transfer(voucherAddress, value)) { + revert("VoucherHub: SRLC transfer to voucher failed"); + } + } } diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index 5bb60ae9..f77cec62 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -10,7 +10,6 @@ import {IexecPoco2} from "@iexec/poco/contracts/modules/interfaces/IexecPoco2.v8 import {IexecPocoAccessors} from "@iexec/poco/contracts/modules/interfaces/IexecPocoAccessors.sol"; import {IexecPocoBoost} from "@iexec/poco/contracts/modules/interfaces/IexecPocoBoost.sol"; import {IexecPocoBoostAccessors} from "@iexec/poco/contracts/modules/interfaces/IexecPocoBoostAccessors.sol"; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import {IVoucherHub} from "../IVoucherHub.sol"; From bc24e63085f4e6a3705da855a07f28e029410fc4 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 17:07:41 +0200 Subject: [PATCH 13/21] refactor transfer funds to voucher --- contracts/VoucherHub.sol | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 92a2d1db..448c7269 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -174,13 +174,10 @@ contract VoucherHub is // The proxy contract does a delegatecall to its implementation. // Re-Entrancy safe because the target contract is controlled. Voucher(voucherAddress).initialize(owner, address(this), expiration, voucherType); - // SRLC - if (!IERC20($._iexecPoco).transfer(voucherAddress, value)) { - revert("VoucherHub: SRLC transfer to voucher failed"); - } _mint(voucherAddress, value); // VCHR $._isVoucher[voucherAddress] = true; emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); + _transfertFundsToVoucher($._iexecPoco, voucherAddress, value); // SRLC } /** @@ -193,10 +190,7 @@ contract VoucherHub is VoucherHubStorage storage $ = _getVoucherHubStorage(); require($._isVoucher[voucher], "VoucherHub: unknown voucher"); _mint(voucher, value); // VCHR - if (!IERC20($._iexecPoco).transfer(voucher, value)) { - // SRLC - revert("VoucherHub: SRLC transfer to voucher failed"); - } + _transfertFundsToVoucher($._iexecPoco, voucher, value); // SRLC uint256 expiration = block.timestamp + $.voucherTypes[Voucher(voucher).getType()].duration; Voucher(voucher).setExpiration(expiration); emit VoucherToppedUp(voucher, expiration, value); From 0fddf5d793a96a5ce410fb8bc34fa353444a5b08 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 17:20:50 +0200 Subject: [PATCH 14/21] remove assembly from exlcuded in config slither --- contracts/VoucherHub.sol | 2 ++ contracts/beacon/Voucher.sol | 2 ++ slither.config.json | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 448c7269..cc6fcd41 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -326,9 +326,11 @@ contract VoucherHub is } function _getVoucherHubStorage() private pure returns (VoucherHubStorage storage $) { + //slither-disable-start assembly assembly { $.slot := VOUCHER_HUB_STORAGE_LOCATION } + //slither-disable-end assembly } function _getCreate2Salt(address account) private pure returns (bytes32) { diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index f77cec62..4a8b5d0f 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -380,9 +380,11 @@ contract Voucher is Initializable, IVoucher { } function _getVoucherStorage() private pure returns (VoucherStorage storage $) { + //slither-disable-start assembly assembly { $.slot := VOUCHER_STORAGE_LOCATION } + //slither-disable-end assembly } // TODO move this function before private view functions. diff --git a/slither.config.json b/slither.config.json index d47b4692..33d6aa91 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,6 +1,6 @@ { "solc_remaps": "@=node_modules/@", "filter_paths": "(mocks/|node_modules/)", - "detectors_to_exclude": "pragma,solc-version,assembly", + "detectors_to_exclude": "pragma,solc-version", "solc_args": "--optimize --via-ir" } From b3f5ac5bbd4d067e6384e0dac95f02646c5e2985 Mon Sep 17 00:00:00 2001 From: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com> Date: Mon, 10 Jun 2024 18:40:31 +0200 Subject: [PATCH 15/21] Apply suggestions from code review Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com> --- contracts/beacon/Voucher.sol | 4 ++-- test/VoucherHub.test.ts | 1 + test/beacon/Voucher.test.ts | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index 4a8b5d0f..70b4a7a8 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -436,8 +436,9 @@ contract Voucher is Initializable, IVoucher { // Transfer non-sponsored amount from the iExec account of the // requester to the iExec account of the voucher //slither-disable-start arbitrary-send-erc20 - // Note: We can disable this check since request order is signed by requester so he agrees to pay for it + // Note: We can disable this check since the requester signed the request order and agreed to pay for the deal. // & caller is only authorized. + // SRLC if ( !IERC20(iexecPoco).transferFrom( requestOrder.requester, @@ -445,7 +446,6 @@ contract Voucher is Initializable, IVoucher { dealPrice - sponsoredAmount ) ) { - // SRLC revert("Voucher: Transfer of non-sponsored amount failed"); } //slither-disable-end arbitrary-send-erc20 diff --git a/test/VoucherHub.test.ts b/test/VoucherHub.test.ts index 1ddf9d14..564289ee 100644 --- a/test/VoucherHub.test.ts +++ b/test/VoucherHub.test.ts @@ -668,6 +668,7 @@ describe('VoucherHub', function () { ), ).to.be.revertedWith('VoucherHub: type index out of bounds'); }); + it('Should not create voucher when SLRC transfer fails', async function () { const { voucherOwner1 } = await loadFixture(deployFixture); await voucherHubWithAssetEligibilityManagerSigner.createVoucherType( diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index df9bf025..d1fa6009 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -1071,6 +1071,7 @@ describe('Voucher', function () { ); }); }); + describe('Should not claim task when SLRC transfer fails', async () => { it('Classic', async () => await runTest(voucherMatchOrders, claim)); it('Boost', async () => await runTest(voucherMatchOrdersBoost, claimBoost)); From c352286b11fbeae915df05d0ef3e4d106804b77c Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 18:41:29 +0200 Subject: [PATCH 16/21] update function name --- contracts/VoucherHub.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index cc6fcd41..a55031e4 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -177,7 +177,7 @@ contract VoucherHub is _mint(voucherAddress, value); // VCHR $._isVoucher[voucherAddress] = true; emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); - _transfertFundsToVoucher($._iexecPoco, voucherAddress, value); // SRLC + _transfertFundsToVoucherOnPoco($._iexecPoco, voucherAddress, value); // SRLC } /** @@ -190,7 +190,7 @@ contract VoucherHub is VoucherHubStorage storage $ = _getVoucherHubStorage(); require($._isVoucher[voucher], "VoucherHub: unknown voucher"); _mint(voucher, value); // VCHR - _transfertFundsToVoucher($._iexecPoco, voucher, value); // SRLC + _transfertFundsToVoucherOnPoco($._iexecPoco, voucher, value); // SRLC uint256 expiration = block.timestamp + $.voucherTypes[Voucher(voucher).getType()].duration; Voucher(voucher).setExpiration(expiration); emit VoucherToppedUp(voucher, expiration, value); @@ -337,7 +337,7 @@ contract VoucherHub is return bytes32(uint256(uint160(account))); } - function _transfertFundsToVoucher( + function _transfertFundsToVoucherOnPoco( address iexecPoco, address voucherAddress, uint256 value From 195fec0890ff736ca8ce529b2499a20c58261638 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 18:42:54 +0200 Subject: [PATCH 17/21] update mock function name --- contracts/mocks/IexecPocoMock.sol | 4 ++-- test/VoucherHub.test.ts | 4 ++-- test/beacon/Voucher.test.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/mocks/IexecPocoMock.sol b/contracts/mocks/IexecPocoMock.sol index 8c07f580..26555b6a 100644 --- a/contracts/mocks/IexecPocoMock.sol +++ b/contracts/mocks/IexecPocoMock.sol @@ -94,11 +94,11 @@ contract IexecPocoMock is ERC20 { shouldRevertOnSponsorMatchOrdersBoost = true; } - function willRevertOnTransfer() external { + function willFailOnTransfer() external { shouldFailOnTransfer = true; } - function willRevertOnTransferFrom() external { + function willFailOnTransferFrom() external { shouldFailOnTransferFrom = true; } diff --git a/test/VoucherHub.test.ts b/test/VoucherHub.test.ts index 564289ee..10913eec 100644 --- a/test/VoucherHub.test.ts +++ b/test/VoucherHub.test.ts @@ -675,7 +675,7 @@ describe('VoucherHub', function () { description, duration, ); - await iexecPocoInstance.willRevertOnTransfer().then((tx) => tx.wait()); + await iexecPocoInstance.willFailOnTransfer().then((tx) => tx.wait()); // Create voucher. await expect( voucherHubWithVoucherManagerSigner.createVoucher( @@ -756,7 +756,7 @@ describe('VoucherHub', function () { }); it('Should not top up when SLRC transfer fails', async function () { const topUpValue = 123n; // arbitrary value - await iexecPocoInstance.willRevertOnTransfer().then((tx) => tx.wait()); + await iexecPocoInstance.willFailOnTransfer().then((tx) => tx.wait()); // Create voucher. await expect( voucherHubWithVoucherManagerSigner.topUpVoucher(voucherAddress, topUpValue), diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index d1fa6009..7dd0b168 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -656,7 +656,7 @@ describe('Voucher', function () { .connect(requester) .approve(await voucherAsOwner.getAddress(), noSponsoredValue) .then((tx) => tx.wait()); - await iexecPocoInstance.willRevertOnTransferFrom().then((tx) => tx.wait()); + await iexecPocoInstance.willFailOnTransferFrom().then((tx) => tx.wait()); await expect( voucherAsOwner.matchOrdersBoost( appOrder, @@ -1088,7 +1088,7 @@ describe('Voucher', function () { .approve(voucherAddress, dealNonSponsoredAmount) .then((tx) => tx.wait()); await matchOrdersBoostOrClassic(); - await iexecPocoInstance.willRevertOnTransfer().then((tx) => tx.wait()); + await iexecPocoInstance.willFailOnTransfer().then((tx) => tx.wait()); await expect(claimBoostOrClassic()).to.be.revertedWith( 'Voucher: transfer to requester failed', ); From b416441a54074e26e64b06ea6238fc4ef69436c6 Mon Sep 17 00:00:00 2001 From: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com> Date: Mon, 10 Jun 2024 18:44:12 +0200 Subject: [PATCH 18/21] Update test/beacon/Voucher.test.ts Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com> --- test/beacon/Voucher.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index 7dd0b168..e250bed6 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -654,7 +654,7 @@ describe('Voucher', function () { await iexecPocoInstance .connect(requester) - .approve(await voucherAsOwner.getAddress(), noSponsoredValue) + .approve(voucherAddress, noSponsoredValue) .then((tx) => tx.wait()); await iexecPocoInstance.willFailOnTransferFrom().then((tx) => tx.wait()); await expect( From 7ac0604952afe25c50b57889ea2e696737787cae Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Mon, 10 Jun 2024 18:48:20 +0200 Subject: [PATCH 19/21] set _iexecPoco in _transfertFundsToVoucherOnPoco function --- contracts/VoucherHub.sol | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index a55031e4..35a80b0e 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -177,7 +177,7 @@ contract VoucherHub is _mint(voucherAddress, value); // VCHR $._isVoucher[voucherAddress] = true; emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); - _transfertFundsToVoucherOnPoco($._iexecPoco, voucherAddress, value); // SRLC + _transfertFundsToVoucherOnPoco(voucherAddress, value); // SRLC } /** @@ -190,7 +190,7 @@ contract VoucherHub is VoucherHubStorage storage $ = _getVoucherHubStorage(); require($._isVoucher[voucher], "VoucherHub: unknown voucher"); _mint(voucher, value); // VCHR - _transfertFundsToVoucherOnPoco($._iexecPoco, voucher, value); // SRLC + _transfertFundsToVoucherOnPoco(voucher, value); // SRLC uint256 expiration = block.timestamp + $.voucherTypes[Voucher(voucher).getType()].duration; Voucher(voucher).setExpiration(expiration); emit VoucherToppedUp(voucher, expiration, value); @@ -337,12 +337,9 @@ contract VoucherHub is return bytes32(uint256(uint160(account))); } - function _transfertFundsToVoucherOnPoco( - address iexecPoco, - address voucherAddress, - uint256 value - ) private { - if (!IERC20(iexecPoco).transfer(voucherAddress, value)) { + function _transfertFundsToVoucherOnPoco(address voucherAddress, uint256 value) private { + VoucherHubStorage storage $ = _getVoucherHubStorage(); + if (!IERC20($._iexecPoco).transfer(voucherAddress, value)) { revert("VoucherHub: SRLC transfer to voucher failed"); } } From 2db6f20fee5f194773359b0df6a59e507656df18 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Tue, 11 Jun 2024 08:49:47 +0200 Subject: [PATCH 20/21] update naming --- contracts/VoucherHub.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index 35a80b0e..f3993d4b 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -177,7 +177,7 @@ contract VoucherHub is _mint(voucherAddress, value); // VCHR $._isVoucher[voucherAddress] = true; emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); - _transfertFundsToVoucherOnPoco(voucherAddress, value); // SRLC + _transferFundsToVoucherOnPoco(voucherAddress, value); // SRLC } /** @@ -190,7 +190,7 @@ contract VoucherHub is VoucherHubStorage storage $ = _getVoucherHubStorage(); require($._isVoucher[voucher], "VoucherHub: unknown voucher"); _mint(voucher, value); // VCHR - _transfertFundsToVoucherOnPoco(voucher, value); // SRLC + _transferFundsToVoucherOnPoco(voucher, value); // SRLC uint256 expiration = block.timestamp + $.voucherTypes[Voucher(voucher).getType()].duration; Voucher(voucher).setExpiration(expiration); emit VoucherToppedUp(voucher, expiration, value); @@ -337,7 +337,7 @@ contract VoucherHub is return bytes32(uint256(uint160(account))); } - function _transfertFundsToVoucherOnPoco(address voucherAddress, uint256 value) private { + function _transferFundsToVoucherOnPoco(address voucherAddress, uint256 value) private { VoucherHubStorage storage $ = _getVoucherHubStorage(); if (!IERC20($._iexecPoco).transfer(voucherAddress, value)) { revert("VoucherHub: SRLC transfer to voucher failed"); From e2fd847ab757baea170cfb286fa4fc62533248ec Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Tue, 11 Jun 2024 09:10:45 +0200 Subject: [PATCH 21/21] added test for both classic and boost + todo --- test/beacon/Voucher.test.ts | 66 ++++++++++++++++++++++++++----------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index e250bed6..cfd858ef 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -10,6 +10,7 @@ import * as commonUtils from '../../scripts/common'; import * as voucherHubUtils from '../../scripts/voucherHubUtils'; import * as voucherUtils from '../../scripts/voucherUtils'; import { + IexecLibOrders_v5, IexecPocoMock, IexecPocoMock__factory, UpgradeableBeacon, @@ -301,6 +302,25 @@ describe('Voucher', function () { const getRequesterBalanceOnIexecPoco = () => iexecPocoInstance.balanceOf(requester.getAddress()); + const voucherMatchOrders = async ( + appOrder: IexecLibOrders_v5.AppOrderStruct, + datasetOrder: IexecLibOrders_v5.DatasetOrderStruct, + workerpoolOrder: IexecLibOrders_v5.WorkerpoolOrderStruct, + requestOrder: IexecLibOrders_v5.RequestOrderStruct, + ) => + await voucherAsOwner.matchOrders(appOrder, datasetOrder, workerpoolOrder, requestOrder); + const voucherMatchOrdersBoost = async ( + appOrder: IexecLibOrders_v5.AppOrderStruct, + datasetOrder: IexecLibOrders_v5.DatasetOrderStruct, + workerpoolOrder: IexecLibOrders_v5.WorkerpoolOrderStruct, + requestOrder: IexecLibOrders_v5.RequestOrderStruct, + ) => + await voucherAsOwner.matchOrdersBoost( + appOrder, + datasetOrder, + workerpoolOrder, + requestOrder, + ); it('Should match orders with full sponsored amount', async () => { await addEligibleAssets([app, dataset, workerpool]); const voucherInitialCreditBalance = await voucherAsOwner.getBalance(); @@ -645,26 +665,32 @@ describe('Voucher', function () { ).to.be.revertedWith('IexecPocoMock: Failed to sponsorMatchOrdersBoost'); }); - it('Should not match orders boost when SRLC transfer fails', async () => { - const noSponsoredValue = appPrice * volume; - await addEligibleAssets([dataset, workerpool]); - await iexecPocoInstance - .transfer(requester, noSponsoredValue) - .then((tx) => tx.wait()); - - await iexecPocoInstance - .connect(requester) - .approve(voucherAddress, noSponsoredValue) - .then((tx) => tx.wait()); - await iexecPocoInstance.willFailOnTransferFrom().then((tx) => tx.wait()); - await expect( - voucherAsOwner.matchOrdersBoost( - appOrder, - datasetOrder, - workerpoolOrder, - requestOrder, - ), - ).to.be.revertedWith('Voucher: Transfer of non-sponsored amount failed'); + //TODO Refactor other tests match orders to both Classic and Boost + describe('Should not match orders when SRLC transfer fails', async () => { + it('Classic', async () => await runTest(voucherMatchOrders)); + it('Boost', async () => await runTest(voucherMatchOrdersBoost)); + + async function runTest(matchOrdersBoostOrClassic: any) { + const noSponsoredValue = appPrice * volume; + await addEligibleAssets([dataset, workerpool]); + await iexecPocoInstance + .transfer(requester, noSponsoredValue) + .then((tx) => tx.wait()); + + await iexecPocoInstance + .connect(requester) + .approve(voucherAddress, noSponsoredValue) + .then((tx) => tx.wait()); + await iexecPocoInstance.willFailOnTransferFrom().then((tx) => tx.wait()); + await expect( + matchOrdersBoostOrClassic( + appOrder, + datasetOrder, + workerpoolOrder, + requestOrder, + ), + ).to.be.revertedWith('Voucher: Transfer of non-sponsored amount failed'); + } }); }); });