Address slither reentrancy findings

* In slither.db.json - updated via slither's triage-mode, but something's bonkers about slither and sometimes things stay ignored, sometimes they don't. Also just running it in normal mode several times will give different results. Therefore, I have no confidence that we'll ever get it to totally shut up, and we should just consider it helpful guide, and not like code coverage output which can eventually reach 100% * In BosonRouter.sol, - removed a lot of whitespace - added virtual keyword to requestCreateOrderETHETH method, so it can be overriden by MockBosonRouter.sol - added nonReentrant modifier to methods: - requestCreateOrderTKNTKNWithPermit - requestCreateOrderTKNTKNWithPermitConditional - requestCreateOrderETHTKNWithPermit - requestCreateOrderETHTKNWithPermitConditional - requestCreateOrderTKNETH - requestCreateOrderTKNETHConditional - removed nonReentrant modifier from requestCreateOrderTKNETHInternal method, which is internal and the modifier has already been invoked on all callers. * In Cashier.sol, - added nonReentrant modifier to methods: - onERC721Transfer - onERC1155Transfer * In DAITokenWrapper.sol, - imported and extended ReentrancyGuard - added nonReentrant modifier to methods: - permit * In ERC1155ERC721.sol, - imported and extended ReentrancyGuard - added nonReentrant modifier to methods: - safeTransferFrom - _transferFrom * In MockBosonRouter.sol, - removed all imports except BosonRouter - extended BosonRouter - added constructor that passes its args to BosonRouter - removed all functions except - requestCreateOrderETHETH (which sends an invalid payment type to requestCreateOrder for a specific test) - transferFromAndAddEscrowTest * In VoucherKernel.sol, - imported and extended ReentrancyGuard - added nonReentrant modifier to methods: - fillOrder - cancelOrFaultVoucherSet This fixes #177
bosonprotocol · Oct 18, 2021 · 280589c · 280589c
1 parent cd15b87
commit 280589c
Show file tree

Hide file tree

Showing 7 changed files with 114 additions and 1,220 deletions.
diff --git a/contracts/BosonRouter.sol b/contracts/BosonRouter.sol
diff --git a/contracts/Cashier.sol b/contracts/Cashier.sol
@@ -911,7 +911,7 @@ contract Cashier is ICashier, UsingHelpers, ReentrancyGuard, Ownable, Pausable {
         address _from,
         address _to,
         uint256 _tokenIdVoucher
-    ) external override onlyTokensContract {
+    ) external override nonReentrant onlyTokensContract {
         address tokenAddress;
 
         uint256 tokenSupplyId =
@@ -1000,7 +1000,7 @@ contract Cashier is ICashier, UsingHelpers, ReentrancyGuard, Ownable, Pausable {
         address _to,
         uint256 _tokenSupplyId,
         uint256 _value
-    ) external override onlyTokensContract {
+    ) external override nonReentrant onlyTokensContract {
         uint8 paymentType =
             IVoucherKernel(voucherKernel).getVoucherPaymentMethod(
                 _tokenSupplyId

diff --git a/contracts/DAITokenWrapper.sol b/contracts/DAITokenWrapper.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: LGPL-3.0-or-later
 pragma solidity 0.7.6;
 
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 import "@openzeppelin/contracts/access/Ownable.sol";
 import "./interfaces/ITokenWrapper.sol";
 import "./interfaces/IDAI.sol";
@@ -9,10 +10,7 @@ import "./interfaces/IDAI.sol";
  * @title DAITokenWrapper
  * @notice Contract for wrapping call to DAI token permit function because the DAI token permit function has a different signature from other tokens with which the protocol integrates
  */
-contract DAITokenWrapper is 
-    ITokenWrapper,
-    Ownable
-{
+contract DAITokenWrapper is ITokenWrapper, Ownable, ReentrancyGuard {
 
     address private daiTokenAddress;
 
@@ -54,6 +52,7 @@ contract DAITokenWrapper is
     ) 
         external
         override
+        nonReentrant
         notZeroAddress(_tokenOwner)
         notZeroAddress(_spender)
     {

diff --git a/contracts/ERC1155ERC721.sol b/contracts/ERC1155ERC721.sol
@@ -2,6 +2,7 @@
 
 pragma solidity 0.7.6;
 
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 import "@openzeppelin/contracts/math/SafeMath.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
 import "@openzeppelin/contracts/access/Ownable.sol";
@@ -17,7 +18,7 @@ import "./interfaces/ICashier.sol";
  * @title Multi-token contract, implementing ERC-1155 and ERC-721 hybrid
  *  Inspired by: https://github.com/pixowl/sandbox-smart-contracts
  */
-contract ERC1155ERC721 is IERC1155ERC721, Ownable {
+contract ERC1155ERC721 is IERC1155ERC721, Ownable, ReentrancyGuard {
     using SafeMath for uint256;
     using Address for address;
 
@@ -78,7 +79,11 @@ contract ERC1155ERC721 is IERC1155ERC721, Ownable {
         uint256 _tokenId,
         uint256 _value,
         bytes calldata _data
-    ) external override {
+    )
+    external
+    override
+    nonReentrant
+    {
         require(_to != address(0), "UNSPECIFIED_ADDRESS");
         require(
             _from == msg.sender || operatorApprovals[_from][msg.sender],
@@ -197,7 +202,7 @@ contract ERC1155ERC721 is IERC1155ERC721, Ownable {
         address _from,
         address _to,
         uint256 _tokenId
-    ) internal {
+    ) internal nonReentrant {
         require(ownerOf(_tokenId) == _from, "UNAUTHORIZED_T");
         require(_to != address(0), "UNSPECIFIED_ADDRESS");
 

diff --git a/contracts/VoucherKernel.sol b/contracts/VoucherKernel.sol
@@ -2,6 +2,7 @@
 
 pragma solidity 0.7.6;
 
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
 import "@openzeppelin/contracts/access/Ownable.sol";
 import "@openzeppelin/contracts/utils/Pausable.sol";
@@ -22,7 +23,7 @@ import "./UsingHelpers.sol";
  *      See: https://ethereum.stackexchange.com/questions/5924/how-do-ethereum-mining-nodes-maintain-a-time-consistent-with-the-network/5931#5931
  */
 // solhint-disable-next-line
-contract VoucherKernel is IVoucherKernel, Ownable, Pausable, UsingHelpers {
+contract VoucherKernel is IVoucherKernel, Ownable, Pausable, ReentrancyGuard, UsingHelpers {
     using Address for address;
     using SafeMath for uint256;
 
@@ -335,7 +336,12 @@ contract VoucherKernel is IVoucherKernel, Ownable, Pausable, UsingHelpers {
         address _issuer,
         address _holder,
         uint8 _paymentMethod
-    ) external override onlyFromRouter {
+    )
+    external
+    override
+    onlyFromRouter
+    nonReentrant
+    {
         uint8 paymentMethod = getVoucherPaymentMethod(_tokenIdSupply);
 
         //checks
@@ -743,11 +749,12 @@ contract VoucherKernel is IVoucherKernel, Ownable, Pausable, UsingHelpers {
      * @param _issuer   owner of the voucher
      */
     function cancelOrFaultVoucherSet(uint256 _tokenIdSupply, address _issuer)
-        external
-        override
-        onlyFromRouter
-        whenNotPaused
-        returns (uint256)
+    external
+    override
+    onlyFromRouter
+    nonReentrant
+    whenNotPaused
+    returns (uint256)
     {
         require(getSupplyHolder(_tokenIdSupply) == _issuer, "UNAUTHORIZED_COF");