Merge pull request #62 from valory-xyz/fix2

refactor and fix: addressing recent changes

audit/internal2/README.md

@@ -48,7 +48,7 @@ next
         } - skip
 5. pass        
 ```
-[]
+[x] Outdated due to design change
 
 #### Medium. _calculatePayment not update collectedFees;
 ```
@@ -57,15 +57,15 @@ function _calculatePayment(
         uint256 payment
     ) internal virtual returns (uint256 mechPayment, uint256 marketplaceFee) {
 ```
-[]
+[x] Outdated due to design change
 
 #### Medium. payable fallback()
 ```
 Must be payable.
 fallback() external {
         // solhint-disable-next-line avoid-low-level-calls
 ```
-[]
+[x] Fixed
 
 #### Medium. Typo in check. 
 ```
@@ -78,10 +78,10 @@ constructor(address _mechMarketplace, address _serviceRegistry, uint256 _service
             revert ZeroValue();
         }
 ```
-[]
+[x] Outdated due to design change
 
 ####  Low? Notices? OlasMech.setUp(bytes) event
-[]
+[x] Noticed, internal storage writing
 
 #### Low? Notices? Karma.sol Uniform approach to location getImplementation() (proxy/implementation)
 ```
@@ -95,7 +95,7 @@ Depending on what they understand better for etherscan. Probably in proxy better
         }
     }
 ```
-[]
+[x] Noticed
 
 ### Low? improvement create2(), due to unpredictability.
 ```
@@ -125,7 +125,7 @@ bytes32 salt = keccak256(abi.encode(nonce, block.timestamp, msg.sender, serviceI
 
 same for contracts\integrations\nevermined\MechFactorySubscription.sol
 ```
-[]
+[x] Fixed
 
 ### Notices
 #### Low? Notices? No issue? initialize and constructor on MechMarketplace + frontrunning (?!) To discussion
@@ -148,7 +148,7 @@ function initialize(uint256 _fee, uint256 _minResponseTimeout, uint256 _maxRespo
 Frontrunning is possible : between constructor() -> initialize() : No issue!
 Changing the storage of implementation has no effect on changing the storage of proxy!
 ```
-[]
+[x] Noticed
 
 #### Low? Notices? No issue? frontrunning initialize() in Karma.sol. To discussion
 ```
@@ -168,22 +168,22 @@ Example:
     }
 Changing the storage of implementation has no effect on changing the storage of proxy!
 ```
-[]
+[x] Noticed, already initialized
 
 ### Notices. Variable "price". Problem of terminology
 ```
 Problem of terminology. Price is usually expressed as amount0/amount1 
 if (amount < price) {}
 ```
-[]
+[x] Outdated
 
 ### Notices. for design createMech()
 ```
 Can be called by anyone, a small limitation is that it is called from the marketPlace?
 function createMech()
 -> callback IMechMarketplace(mechMarketplace).mapMechFactories[mechFactory] == address(this)
 ```
-[]
+[x] Fixed
 
 ### Notices. Pure? _calculatePayment()
 ```
@@ -193,12 +193,12 @@ function _calculatePayment(
     ) internal virtual returns (uint256 mechPayment, uint256 marketplaceFee)
     -> pure?
 ```
-[]
+[x] Outdated
 
 ### Notices. Low/Notices
 ```
 uint256 private constant FEE_BASIS_POINTS = 10_000; // 100% в bps
 ```
-[]
+[x] Fixed
 
 

audit/internal3/README.md

@@ -20,28 +20,29 @@ Most of the issues raised by instrumental analysis are outside the scope of the
 
 ### Issue
 #### Medium. Make sure that the previous comments (internal - internal2) have been addressed.
-[]
+[x] Addressed
 
 #### Critical/Medium. checkAndRecordDeliveryRate() for native token
 ```
 function checkAndRecordDeliveryRate()
 This is not the right behavior for a case msg.value > 0 and native token contracts.
 ```
+[x] Fixed
 
 #### Low. More Reentrancy protection
 ```
 function checkAndRecordDeliveryRate()
 function finalizeDeliveryRate() 
 + Reentrancy protection agains IMech(mech).maxDeliveryRate()
 ```
-[]
+[x] Fixed
 
 #### Notices. unintuitive name vs action
 ```
 function _checkNativeValue() internal virtual override
 The function name is very unintuitive. In reality, it just does a revert if the contract type is token.
 ```
-[]
+[x] Fixed
 
 #### Notices. checkAndRecordDeliveryRate why are uint256 parameters needed
 ```
@@ -53,4 +54,4 @@ Why unused params?
         uint256
 If this is abstract data for the future, it is better to replace it with bytes payload.
 ```
-[]
+[x] Fixed

contracts/BalanceTrackerFixedPriceBase.sol

@@ -82,7 +82,7 @@ abstract contract BalanceTrackerFixedPriceBase {
         buyBackBurner = _buyBackBurner;
     }
 
-    function _checkNativeValue() internal virtual;
+    function _getOrRestrictNativeValue() internal virtual returns (uint256);
 
     function _getRequiredFunds(address requester, uint256 balanceDiff) internal virtual returns (uint256);
 
@@ -92,26 +92,26 @@ abstract contract BalanceTrackerFixedPriceBase {
         address requester,
         bytes memory
     ) external payable {
+        // Reentrancy guard
+        if (_locked > 1) {
+            revert ReentrancyGuard();
+        }
+        _locked = 2;
+
         // Check for marketplace access
         if (msg.sender != mechMarketplace) {
             revert MarketplaceOnly(msg.sender, mechMarketplace);
         }
 
         // Check for native value
-        _checkNativeValue();
+        uint256 initAmount = _getOrRestrictNativeValue();
+
+        // Get account balance
+        uint256 balance = mapRequesterBalances[requester] + initAmount;
 
         // Get mech max delivery rate
         uint256 maxDeliveryRate = IMech(mech).maxDeliveryRate();
 
-        // Get account balance
-        uint256 balance = mapRequesterBalances[requester];
-
-        // Update balance with native value
-        if (msg.value > 0) {
-            balance += msg.value;
-            emit Deposit(msg.sender, address(0), msg.value);
-        }
-
         // Check the request delivery rate for a fixed price
         if (balance < maxDeliveryRate) {
             // Get balance difference
@@ -127,6 +127,8 @@ abstract contract BalanceTrackerFixedPriceBase {
         // Adjust account balance
         balance -= maxDeliveryRate;
         mapRequesterBalances[requester] = balance;
+
+        _locked = 1;
     }
 
     /// @dev Finalizes mech delivery rate based on requested and actual ones.

contracts/BalanceTrackerFixedPriceNative.sol

@@ -35,7 +35,14 @@ contract BalanceTrackerFixedPriceNative is BalanceTrackerFixedPriceBase {
         wrappedNativeToken = _wrappedNativeToken;
     }
 
-    function _checkNativeValue() internal virtual override {}
+    function _getOrRestrictNativeValue() internal virtual override returns (uint256) {
+        // Update balance with native value
+        if (msg.value > 0) {
+            emit Deposit(msg.sender, address(0), msg.value);
+        }
+
+        return msg.value;
+    }
 
     function _getRequiredFunds(address, uint256) internal virtual override returns (uint256) {
         return 0;

contracts/BalanceTrackerFixedPriceToken.sol

@@ -17,6 +17,11 @@ interface IToken {
     /// @param amount Amount to transfer to.
     /// @return True if the function execution is successful.
     function transferFrom(address from, address to, uint256 amount) external returns (bool);
+
+    /// @dev Gets the amount of tokens owned by a specified account.
+    /// @param account Account address.
+    /// @return Amount of tokens owned.
+    function balanceOf(address account) external view returns (uint256);
 }
 
 contract BalanceTrackerFixedPriceNative is BalanceTrackerFixedPriceBase {
@@ -38,16 +43,29 @@ contract BalanceTrackerFixedPriceNative is BalanceTrackerFixedPriceBase {
         olas = _olas;
     }
 
-    function _checkNativeValue() internal virtual override {
+    function _getOrRestrictNativeValue() internal virtual override returns (uint256) {
         // Check for msg.value
         if (msg.value > 0) {
             revert NoDepositAllowed(msg.value);
         }
+
+        return 0;
     }
 
     function _getRequiredFunds(address requester, uint256 balanceDiff) internal virtual override returns (uint256) {
-        // TODO check balances before and after
+        uint256 balanceBefore = IToken(olas).balanceOf(address(this));
+        // Get tokens from requester
         IToken(olas).transferFrom(requester, address(this), balanceDiff);
+        uint256 balanceAfter = IToken(olas).balanceOf(address(this));
+
+        // Check the balance
+        uint256 diff = balanceAfter - balanceBefore;
+        if (diff != balanceDiff) {
+            revert TransferFailed(olas, requester, address(this), balanceDiff);
+        }
+
+        emit Deposit(msg.sender, olas, balanceDiff);
+
         return balanceDiff;
     }
 

contracts/MechFactoryFixedPriceNative.sol

@@ -8,12 +8,22 @@ import {MechFixedPriceNative} from "./MechFixedPriceNative.sol";
 /// @param expected Expected data length.
 error IncorrectDataLength(uint256 provided, uint256 expected);
 
+/// @dev Only `marketplace` has a privilege, but the `sender` was provided.
+/// @param sender Sender address.
+/// @param marketplace Required marketplace address.
+error MarketplaceOnly(address sender, address marketplace);
+
+/// @dev Provided zero address.
+error ZeroAddress();
+
 /// @title Mech Factory Basic - Periphery smart contract for managing basic mech creation
 contract MechFactoryFixedPriceNative {
     event CreateFixedPriceMech(address indexed mech, uint256 indexed serviceId, uint256 maxDeliveryRate);
 
     // Agent factory version number
     string public constant VERSION = "0.1.0";
+    // Nonce
+    uint256 internal _nonce;
 
     /// @dev Registers service as a mech.
     /// @param mechMarketplace Mech marketplace address.
@@ -27,7 +37,10 @@ contract MechFactoryFixedPriceNative {
         uint256 serviceId,
         bytes memory payload
     ) external returns (address mech) {
-        // TODO: restrict all factories to be called from marketplace only - makes it easier to monitor the system
+        // Check for marketplace access
+        if (msg.sender != mechMarketplace) {
+            revert MarketplaceOnly(msg.sender, mechMarketplace);
+        }
 
         // Check payload length
         if (payload.length != 32) {
@@ -37,12 +50,19 @@ contract MechFactoryFixedPriceNative {
         // Decode max delivery rate
         uint256 maxDeliveryRate = abi.decode(payload, (uint256));
 
+        uint256 localNonce = _nonce;
         // Get salt
-        bytes32 salt = keccak256(abi.encode(block.timestamp, msg.sender, serviceId));
+        bytes32 salt = keccak256(abi.encode(block.timestamp, msg.sender, serviceId, localNonce));
+        _nonce = localNonce + 1;
 
         // Service multisig is isOperator() for the mech
         mech = address((new MechFixedPriceNative){salt: salt}(mechMarketplace, serviceRegistry, serviceId, maxDeliveryRate));
 
+        // Check for zero address
+        if (mech == address(0)) {
+            revert ZeroAddress();
+        }
+
         emit CreateFixedPriceMech(mech, serviceId, maxDeliveryRate);
     }
 }

contracts/MechFactoryFixedPriceToken.sol

@@ -8,12 +8,22 @@ import {MechFixedPriceToken} from "./MechFixedPriceToken.sol";
 /// @param expected Expected data length.
 error IncorrectDataLength(uint256 provided, uint256 expected);
 
+/// @dev Only `marketplace` has a privilege, but the `sender` was provided.
+/// @param sender Sender address.
+/// @param marketplace Required marketplace address.
+error MarketplaceOnly(address sender, address marketplace);
+
+/// @dev Provided zero address.
+error ZeroAddress();
+
 /// @title Mech Factory Basic - Periphery smart contract for managing basic mech creation
 contract MechFactoryFixedPriceNative {
     event CreateFixedPriceMech(address indexed mech, uint256 indexed serviceId, uint256 maxDeliveryRate);
 
     // Agent factory version number
     string public constant VERSION = "0.1.0";
+    // Nonce
+    uint256 internal _nonce;
 
     /// @dev Registers service as a mech.
     /// @param mechMarketplace Mech marketplace address.
@@ -27,6 +37,11 @@ contract MechFactoryFixedPriceNative {
         uint256 serviceId,
         bytes memory payload
     ) external returns (address mech) {
+        // Check for marketplace access
+        if (msg.sender != mechMarketplace) {
+            revert MarketplaceOnly(msg.sender, mechMarketplace);
+        }
+
         // Check payload length
         if (payload.length != 32) {
             revert IncorrectDataLength(payload.length, 32);
@@ -35,12 +50,19 @@ contract MechFactoryFixedPriceNative {
         // Decode max delivery rate
         uint256 maxDeliveryRate = abi.decode(payload, (uint256));
 
+        uint256 localNonce = _nonce;
         // Get salt
-        bytes32 salt = keccak256(abi.encode(block.timestamp, msg.sender, serviceId));
+        bytes32 salt = keccak256(abi.encode(block.timestamp, msg.sender, serviceId, localNonce));
+        _nonce = localNonce + 1;
 
         // Service multisig is isOperator() for the mech
         mech = address((new MechFixedPriceToken){salt: salt}(mechMarketplace, serviceRegistry, serviceId, maxDeliveryRate));
 
+        // Check for zero address
+        if (mech == address(0)) {
+            revert ZeroAddress();
+        }
+
         emit CreateFixedPriceMech(mech, serviceId, maxDeliveryRate);
     }
 }

contracts/MechFixedPriceNative.sol

@@ -5,13 +5,16 @@ import {OlasMech} from "./OlasMech.sol";
 
 /// @title MechFixedPriceNative - Smart contract for OlasMech that accepts a fixed price payment for services in native token.
 contract MechFixedPriceNative is OlasMech {
+    // keccak256(FixedPriceNative) = ba699a34be8fe0e7725e93dcbce1701b0211a8ca61330aaeb8a05bf2ec7abed1
+    bytes32 public constant PAYMENT_TYPE = 0xba699a34be8fe0e7725e93dcbce1701b0211a8ca61330aaeb8a05bf2ec7abed1;
+
     /// @dev AgentMech constructor.
     /// @param _mechMarketplace Mech marketplace address.
     /// @param _serviceRegistry Address of the token contract.
     /// @param _serviceId Service Id.
     /// @param _maxDeliveryRate The maximum delivery rate.
     constructor(address _mechMarketplace, address _serviceRegistry, uint256 _serviceId, uint256 _maxDeliveryRate)
-        OlasMech(_mechMarketplace, _serviceRegistry, _serviceId, _maxDeliveryRate, PaymentType.FixedPriceNative)
+        OlasMech(_mechMarketplace, _serviceRegistry, _serviceId, _maxDeliveryRate, PAYMENT_TYPE)
     {}
 
     /// @dev Performs actions before the delivery of a request.