Analyse slither (#26)

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)

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";
@@ -86,12 +85,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(
@@ -172,10 +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);
-        IERC20($._iexecPoco).transfer(voucherAddress, value); // SRLC
         _mint(voucherAddress, value); // VCHR
         $._isVoucher[voucherAddress] = true;
         emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value);
+        _transferFundsToVoucherOnPoco(voucherAddress, value); // SRLC
     }
 
     /**
@@ -188,7 +190,7 @@ contract VoucherHub is
         VoucherHubStorage storage $ = _getVoucherHubStorage();
         require($._isVoucher[voucher], "VoucherHub: unknown voucher");
         _mint(voucher, value); // VCHR
-        IERC20($._iexecPoco).transfer(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);
@@ -324,12 +326,21 @@ 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) {
         return bytes32(uint256(uint160(account)));
     }
+
+    function _transferFundsToVoucherOnPoco(address voucherAddress, uint256 value) private {
+        VoucherHubStorage storage $ = _getVoucherHubStorage();
+        if (!IERC20($._iexecPoco).transfer(voucherAddress, value)) {
+            revert("VoucherHub: SRLC transfer to voucher failed");
+        }
+    }
 }

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";
@@ -373,15 +372,19 @@ 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");
+                }
             }
         }
     }
 
     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.
@@ -432,11 +435,20 @@ 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
-            );
+            //slither-disable-start arbitrary-send-erc20
+            // 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,
+                    address(this),
+                    dealPrice - sponsoredAmount
+                )
+            ) {
+                revert("Voucher: Transfer of non-sponsored amount failed");
+            }
+            //slither-disable-end arbitrary-send-erc20
         }
     }
 }

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 shouldFailOnTransfer = false;
+    bool public shouldFailOnTransferFrom = false;
 
     bytes32 public mockDealId = keccak256("deal");
     uint256 public mockTaskIndex = 0;
@@ -92,6 +94,14 @@ contract IexecPocoMock is ERC20 {
         shouldRevertOnSponsorMatchOrdersBoost = true;
     }
 
+    function willFailOnTransfer() external {
+        shouldFailOnTransfer = true;
+    }
+
+    function willFailOnTransferFrom() external {
+        shouldFailOnTransferFrom = 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 (shouldFailOnTransfer) {
+            return false;
+        }
+        return super.transfer(recipient, amount);
+    }
+
+    function transferFrom(
+        address sender,
+        address recipient,
+        uint256 amount
+    ) public override returns (bool) {
+        if (shouldFailOnTransferFrom) {
+            return false;
+        }
+        return super.transferFrom(sender, recipient, amount);
+    }
 }

slither.config.json

@@ -1,5 +1,6 @@
 {
     "solc_remaps": "@=node_modules/@",
     "filter_paths": "(mocks/|node_modules/)",
+    "detectors_to_exclude": "pragma,solc-version",
     "solc_args": "--optimize --via-ir"
 }

test/VoucherHub.test.ts

@@ -668,6 +668,23 @@ 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.willFailOnTransfer().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 +754,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.willFailOnTransfer().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 () {

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();
@@ -644,6 +664,34 @@ describe('Voucher', function () {
                     ),
                 ).to.be.revertedWith('IexecPocoMock: Failed to sponsorMatchOrdersBoost');
             });
+
+            //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');
+                }
+            });
         });
     });
 
@@ -1049,6 +1097,29 @@ 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.willFailOnTransfer().then((tx) => tx.wait());
+                await expect(claimBoostOrClassic()).to.be.revertedWith(
+                    'Voucher: transfer to requester failed',
+                );
+            }
+        });
     });
 
     async function addEligibleAssets(assets: string[]) {