diff --git a/audit/internal2/README.md b/audit/internal2/README.md index 5123c60..52509dc 100644 --- a/audit/internal2/README.md +++ b/audit/internal2/README.md @@ -48,7 +48,7 @@ next } - skip 5. pass ``` -[] +[x] Outdated due to design change #### Medium. _calculatePayment not update collectedFees; ``` @@ -57,7 +57,7 @@ function _calculatePayment( uint256 payment ) internal virtual returns (uint256 mechPayment, uint256 marketplaceFee) { ``` -[] +[x] Outdated due to design change #### Medium. payable fallback() ``` @@ -65,7 +65,7 @@ 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,14 +168,14 @@ 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() ``` @@ -183,7 +183,7 @@ Can be called by anyone, a small limitation is that it is called from the market 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 diff --git a/audit/internal3/README.md b/audit/internal3/README.md index 5c3fd22..2188b32 100644 --- a/audit/internal3/README.md +++ b/audit/internal3/README.md @@ -20,13 +20,14 @@ 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 ``` @@ -34,14 +35,14 @@ 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. ``` -[] \ No newline at end of file +[x] Fixed \ No newline at end of file diff --git a/contracts/BalanceTrackerFixedPriceBase.sol b/contracts/BalanceTrackerFixedPriceBase.sol index fec502e..d177c84 100644 --- a/contracts/BalanceTrackerFixedPriceBase.sol +++ b/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. diff --git a/contracts/BalanceTrackerFixedPriceNative.sol b/contracts/BalanceTrackerFixedPriceNative.sol index d383196..067f1ee 100644 --- a/contracts/BalanceTrackerFixedPriceNative.sol +++ b/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; diff --git a/contracts/BalanceTrackerFixedPriceToken.sol b/contracts/BalanceTrackerFixedPriceToken.sol index 048ddf8..e446525 100644 --- a/contracts/BalanceTrackerFixedPriceToken.sol +++ b/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; } diff --git a/contracts/MechFactoryFixedPriceNative.sol b/contracts/MechFactoryFixedPriceNative.sol index e9c8eae..33653f9 100644 --- a/contracts/MechFactoryFixedPriceNative.sol +++ b/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); } } diff --git a/contracts/MechFactoryFixedPriceToken.sol b/contracts/MechFactoryFixedPriceToken.sol index 5039915..3436b8e 100644 --- a/contracts/MechFactoryFixedPriceToken.sol +++ b/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); } } diff --git a/contracts/MechFixedPriceNative.sol b/contracts/MechFixedPriceNative.sol index d4fb65b..963f074 100644 --- a/contracts/MechFixedPriceNative.sol +++ b/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. diff --git a/contracts/MechFixedPriceToken.sol b/contracts/MechFixedPriceToken.sol index 3b44018..6144d21 100644 --- a/contracts/MechFixedPriceToken.sol +++ b/contracts/MechFixedPriceToken.sol @@ -5,13 +5,16 @@ import {OlasMech} from "./OlasMech.sol"; /// @title MechFixedPriceToken - Smart contract for OlasMech that accepts a fixed price payment for services in native token. contract MechFixedPriceToken is OlasMech { + // keccak256(FixedPriceToken) = 3679d66ef546e66ce9057c4a052f317b135bc8e8c509638f7966edfd4fcf45e9 + bytes32 public constant PAYMENT_TYPE = 0x3679d66ef546e66ce9057c4a052f317b135bc8e8c509638f7966edfd4fcf45e9; + /// @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.FixedPriceToken) + OlasMech(_mechMarketplace, _serviceRegistry, _serviceId, _maxDeliveryRate, PAYMENT_TYPE) {} /// @dev Performs actions before the delivery of a request. diff --git a/contracts/MechMarketplace.sol b/contracts/MechMarketplace.sol index 9c40194..5afea9e 100644 --- a/contracts/MechMarketplace.sol +++ b/contracts/MechMarketplace.sol @@ -32,7 +32,7 @@ struct MechDelivery { uint256 deliveryRate; } -/// @title Mech Marketplace - Marketplace for posting and delivering requests served by agent mechs +/// @title Mech Marketplace - Marketplace for posting and delivering requests served by mechs /// @author Aleksandr Kuperman - /// @author Andrey Lebedev - /// @author Silvere Gangloff - @@ -42,7 +42,7 @@ contract MechMarketplace is IErrorsMarketplace { event ImplementationUpdated(address indexed implementation); event MarketplaceParamsUpdated(uint256 fee, uint256 minResponseTimeout, uint256 maxResponseTimeout); event SetMechFactoryStatuses(address[] mechFactories, bool[] statuses); - event SetPaymentTypeBalanceTrackers(uint8[] paymentTypes, address[] balanceTrackers); + event SetPaymentTypeBalanceTrackers(bytes32[] paymentTypes, address[] balanceTrackers); event MarketplaceRequest(address indexed requester, address indexed requestedMech, uint256 requestId, bytes data); event MarketplaceDeliver(address indexed priorityMech, address indexed actualMech, address indexed requester, uint256 requestId, bytes data); @@ -89,13 +89,12 @@ contract MechMarketplace is IErrorsMarketplace { // Contract owner address public owner; - // TODO: Aren't we duplicating info hereand on mech re the address specific counts? // Map of request counts for corresponding requester mapping(address => uint256) public mapRequestCounts; // Map of delivery counts for corresponding requester mapping(address => uint256) public mapDeliveryCounts; // Map of delivery counts for mechs - mapping(address => uint256) public mapAgentMechDeliveryCounts; + mapping(address => uint256) public mapMechDeliveryCounts; // Map of delivery counts for corresponding mech service multisig mapping(address => uint256) public mapMechServiceDeliveryCounts; // Mapping of request Id => mech delivery information @@ -104,8 +103,8 @@ contract MechMarketplace is IErrorsMarketplace { mapping(address => bool) public mapMechFactories; // Map of mech => its creating factory mapping(address => address) public mapAgentMechFactories; - // Map of mech type => balanceTracker address - mapping(uint8 => address) public mapPaymentTypeBalanceTrackers; + // Map of payment type => balanceTracker address + mapping(bytes32 => address) public mapPaymentTypeBalanceTrackers; // Mapping of account nonces mapping(address => uint256) public mapNonces; // Mapping of service ids to mechs @@ -307,7 +306,7 @@ contract MechMarketplace is IErrorsMarketplace { /// @dev Sets mech payment type balanceTrackers. /// @param paymentTypes Mech types. /// @param balanceTrackers Corresponding balanceTracker addresses. - function setPaymentTypeBalanceTrackers(uint8[] memory paymentTypes, address[] memory balanceTrackers) external { + function setPaymentTypeBalanceTrackers(bytes32[] memory paymentTypes, address[] memory balanceTrackers) external { // Check for the ownership if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); @@ -378,7 +377,7 @@ contract MechMarketplace is IErrorsMarketplace { requestId = getRequestId(msg.sender, data, mapNonces[msg.sender]); // Get balance tracker address - uint8 mechPaymentType = IMech(priorityMech).getPaymentType(); + bytes32 mechPaymentType = IMech(priorityMech).paymentType(); address balanceTracker = mapPaymentTypeBalanceTrackers[mechPaymentType]; // Check and record mech delivery rate @@ -423,7 +422,7 @@ contract MechMarketplace is IErrorsMarketplace { /// @param requestData Self-descriptive opaque data-blob. function deliverMarketplace( uint256 requestId, - bytes memory requestData, + bytes memory requestData ) external { // Reentrancy guard if (_locked > 1) { @@ -468,9 +467,9 @@ contract MechMarketplace is IErrorsMarketplace { // Decrease the number of undelivered requests numUndeliveredRequests--; // Increase the amount of requester delivered requests - mapDeliveryCounts[requester]++; + mapDeliveryCounts[mechDelivery.requester]++; // Increase the amount of mech delivery counts - mapAgentMechDeliveryCounts[msg.sender]++; + mapMechDeliveryCounts[msg.sender]++; // Increase the amount of mech service multisig delivered requests mapMechServiceDeliveryCounts[mechServiceMultisig]++; @@ -478,14 +477,14 @@ contract MechMarketplace is IErrorsMarketplace { IKarma(karma).changeMechKarma(msg.sender, 1); // Get balance tracker address - uint8 mechPaymentType = IMech(priorityMech).getPaymentType(); + bytes32 mechPaymentType = IMech(priorityMech).paymentType(); address balanceTracker = mapPaymentTypeBalanceTrackers[mechPaymentType]; // Process payment IBalanceTracker(balanceTracker).finalizeDeliveryRate(msg.sender, mechDelivery.requester, requestId, mechDelivery.deliveryRate); - emit MarketplaceDeliver(priorityMech, msg.sender, requester, requestId, requestData); + emit MarketplaceDeliver(priorityMech, msg.sender, mechDelivery.requester, requestId, requestData); _locked = 1; } @@ -537,17 +536,9 @@ contract MechMarketplace is IErrorsMarketplace { } /// @dev Checks for mech validity. - /// @dev mech Agent mech contract address. - /// @param mechServiceId Agent mech service Id. - /// @return multisig - function checkMech( - address mech, - ) public view returns (address multisig) { - // Check for zero value - if (mechServiceId == 0) { - revert ZeroValue(); - } - + /// @param mech Mech contract address. + /// @return multisig Mech service multisig address. + function checkMech(address mech) public view returns (address multisig){ uint256 mechServiceId = IMech(mech).tokenId(); // Check mech validity as it must be created and recorded via this marketplace @@ -555,7 +546,7 @@ contract MechMarketplace is IErrorsMarketplace { revert UnauthorizedAccount(mech); } - // Check mech service Id, if applicable + // Check mech service Id multisig = checkServiceAndGetMultisig(mechServiceId); // Check that service multisig is the priority mech service multisig @@ -566,7 +557,7 @@ contract MechMarketplace is IErrorsMarketplace { /// @dev Checks for requester validity. /// @notice Explicitly allows for EOAs without service id. - /// @dev requester Requester contract address. + /// @param requester Requester address. /// @param requesterServiceId Requester service Id. function checkRequester( address requester, @@ -602,41 +593,5 @@ contract MechMarketplace is IErrorsMarketplace { } } } - - /// @dev Gets the requests count for a specific account. - /// @param account Account address. - /// @return Requests count. - function getRequestsCount(address account) external view returns (uint256) { - return mapRequestCounts[account]; - } - - /// @dev Gets the deliveries count for a specific account. - /// @param account Account address. - /// @return Deliveries count. - function getDeliveriesCount(address account) external view returns (uint256) { - return mapDeliveryCounts[account]; - } - - // TODO Check if needed - /// @dev Gets deliveries count for a specific mech. - /// @param agentMech Agent mech address. - /// @return Deliveries count. - function getAgentMechDeliveriesCount(address agentMech) external view returns (uint256) { - return mapAgentMechDeliveryCounts[agentMech]; - } - - /// @dev Gets deliveries count for a specific mech service multisig. - /// @param mechServiceMultisig Agent mech service multisig address. - /// @return Deliveries count. - function getMechServiceDeliveriesCount(address mechServiceMultisig) external view returns (uint256) { - return mapMechServiceDeliveryCounts[mechServiceMultisig]; - } - - /// @dev Gets mech delivery info. - /// @param requestId Request Id. - /// @return Mech delivery info. - function getMechDeliveryInfo(uint256 requestId) external view returns (MechDelivery memory) { - return mapRequestIdDeliveries[requestId]; - } } diff --git a/contracts/OlasMech.sol b/contracts/OlasMech.sol index 12c30df..f36b498 100644 --- a/contracts/OlasMech.sol +++ b/contracts/OlasMech.sol @@ -14,12 +14,6 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { event Request(address indexed sender, uint256 requestId, bytes data); event RevokeRequest(address indexed sender, uint256 requestId); - enum PaymentType { // this level of the contract should not have to specify its possible extensions. So Payment type needs to be something that's extensible, string or number.. - FixedPriceNative, - FixedPriceToken, - NvmSubscription - } - enum RequestStatus { DoesNotExist, Requested, @@ -38,8 +32,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { // Mech marketplace address address public immutable mechMarketplace; // Mech payment type - // TODO - can it be id or string so its' forward compatible? - PaymentType public immutable paymentType; + bytes32 public immutable paymentType; // TODO give it a better name // Maximum required delivery rate @@ -53,20 +46,12 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { // Reentrancy lock uint256 internal _locked = 1; - // TODO do we need the next two maps? Is it for staking activity checks? - // Map of request counts for corresponding addresses in this agent mech - mapping(address => uint256) public mapRequestCounts; - // Map of delivery counts for corresponding addresses in this agent mech - mapping(address => uint256) public mapDeliveryCounts; // Map of undelivered requests counts for corresponding addresses in this agent mech mapping(address => uint256) public mapUndeliveredRequestsCounts; // Cyclical map of request Ids mapping(uint256 => uint256[2]) public mapRequestIds; // Map of request Id => sender address mapping(uint256 => address) public mapRequestAddresses; - // TODO is a global nonce not sufficient? - // Mapping of account nonces - mapping(address => uint256) public mapNonces; /// @param _mechMarketplace Mech marketplace address. /// @param _serviceRegistry Address of the registry contract. @@ -75,8 +60,8 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { address _mechMarketplace, address _serviceRegistry, uint256 _serviceId, - uint256 _maxDeliveryRate, // TODO - for subscriptions, why do we even specify this here; it should be specified by the requester - PaymentType _paymentType + uint256 _maxDeliveryRate, + bytes32 _paymentType ) { // Check for zero address if (mechMarketplace == address(0)) { @@ -89,7 +74,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { } // Check for zero value - if (_serviceId == 0 || _maxDeliveryRate == 0) { + if (_serviceId == 0 || _maxDeliveryRate == 0 || _paymentType == 0) { revert ZeroValue(); } @@ -112,7 +97,6 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { maxDeliveryRate = _maxDeliveryRate; paymentType = _paymentType; - // Record chain Id chainId = block.chainid; // Compute domain separator @@ -133,9 +117,9 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { ); } - // TODO TBD + // TODO Rename account for requester /// @dev Performs actions before the delivery of a request. - /// @param account Request sender address. + /// @param account Requester address. /// @param requestId Request Id. /// @param data Self-descriptive opaque data-blob. /// @return requestData Data for the request processing. @@ -146,7 +130,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { ) internal virtual returns (bytes memory requestData); /// @dev Registers a request. - /// @param account Requester account address. + /// @param account Requester address. /// @param requestId Request Id. /// @param data Self-descriptive opaque data-blob. function _request( @@ -154,8 +138,6 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { uint256 requestId, bytes memory data ) internal virtual { - // Increase the requests count supplied by the sender - mapRequestCounts[account]++; mapUndeliveredRequestsCounts[account]++; // Record the requestId => sender correspondence mapRequestAddresses[requestId] = account; @@ -223,7 +205,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { } // Check for max delivery rate compared to requested one - if (maxDeliveryRate < mechDelivery.deliveryRate) { + if (maxDeliveryRate > mechDelivery.deliveryRate) { revert Overflow(maxDeliveryRate, mechDelivery.deliveryRate); } @@ -244,7 +226,6 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { requestData = _preDeliver(account, requestId, data); // Increase the total number of deliveries, as the request is delivered by this mech - mapDeliveryCounts[account]++; numTotalDeliveries++; emit Deliver(msg.sender, requestId, requestData); @@ -304,7 +285,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { /// @param data Self-descriptive opaque data-blob. function deliverToMarketplace( uint256 requestId, - bytes memory data, + bytes memory data ) external onlyOperator { // Reentrancy guard if (_locked > 1) { @@ -317,8 +298,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { // Mech marketplace delivery finalization if the request was not delivered already if (requestData.length > 0) { - IMechMarketplace(mechMarketplace).deliverMarketplace(requestId, requestData, - tokenId()); + IMechMarketplace(mechMarketplace).deliverMarketplace(requestId, requestData, tokenId()); } _locked = 1; @@ -345,11 +325,11 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { ); (, address multisig, , , , , IServiceRegistry.ServiceState state) = - IServiceRegistry(_serviceRegistry).mapServices(tokenId()); + IServiceRegistry(serviceRegistry).mapServices(serviceId); // Check for correct service state if (state != IServiceRegistry.ServiceState.Deployed) { - revert WrongServiceState(uint256(state), tokenId()); + revert WrongServiceState(uint256(state), serviceId); } return multisig == signer; } @@ -385,20 +365,6 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { )); } - /// @dev Gets the requests count for a specific account. - /// @param account Account address. - /// @return Requests count. - function getRequestsCount(address account) external view returns (uint256) { - return mapRequestCounts[account]; - } - - /// @dev Gets the deliveries count for a specific account. - /// @param account Account address. - /// @return Deliveries count. - function getDeliveriesCount(address account) external view returns (uint256) { - return mapDeliveryCounts[account]; - } - /// @dev Gets the request Id status registered in this agent mech. /// @notice If marketplace is not zero, use the same function in the mech marketplace contract. /// @param requestId Request Id. @@ -456,11 +422,6 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { } } - - function getPaymentType() external view returns (uint8) { - return uint8(paymentType); - } - /// @dev Gets finalized delivery rate for a request Id. /// @param requestId Request Id. /// @return Finalized delivery rate. diff --git a/contracts/integrations/nevermined/MechFactoryNvmSubscription.sol b/contracts/integrations/nevermined/MechFactoryNvmSubscription.sol index 2b5d63a..076c14c 100644 --- a/contracts/integrations/nevermined/MechFactoryNvmSubscription.sol +++ b/contracts/integrations/nevermined/MechFactoryNvmSubscription.sol @@ -8,12 +8,22 @@ import {MechNvmSubscription} from "./MechNvmSubscription.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 MechFactoryNvmSubscription - Periphery smart contract for managing Nevermined subscription mech creation contract MechFactoryNvmSubscription { event CreateSubscriptionMech(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 MechFactoryNvmSubscription { 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,13 +50,20 @@ contract MechFactoryNvmSubscription { // Decode subscription parameters 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 MechNvmSubscription){salt: salt}(mechMarketplace, serviceRegistry, serviceId, maxDeliveryRate)); + // Check for zero address + if (mech == address(0)) { + revert ZeroAddress(); + } + emit CreateSubscriptionMech(mech, serviceId, maxDeliveryRate); } } diff --git a/contracts/integrations/nevermined/MechNvmSubscription.sol b/contracts/integrations/nevermined/MechNvmSubscription.sol index 18fc59f..d943e5e 100644 --- a/contracts/integrations/nevermined/MechNvmSubscription.sol +++ b/contracts/integrations/nevermined/MechNvmSubscription.sol @@ -28,6 +28,9 @@ error ZeroValue(); contract MechNvmSubscription is OlasMech { event RequestRateFinalized(uint256 indexed requestId, uint256 deliveryRate); + // keccak256(NvmSubscription) = 626e3e03bc0d3f35fa97066f92f71221d599a2bcf50a2c9d6cfa6572204006a0 + bytes32 public constant PAYMENT_TYPE = 0x626e3e03bc0d3f35fa97066f92f71221d599a2bcf50a2c9d6cfa6572204006a0; + // Mapping for requestId => finalized delivery rates mapping(uint256 => uint256) public mapRequestIdFinalizedRates; @@ -42,7 +45,7 @@ contract MechNvmSubscription is OlasMech { uint256 _serviceId, uint256 _maxDeliveryRate ) - OlasMech(_mechMarketplace, _serviceRegistry, _serviceId, _maxDeliveryRate, PaymentType.NvmSubscription) + OlasMech(_mechMarketplace, _serviceRegistry, _serviceId, _maxDeliveryRate, PAYMENT_TYPE) {} /// @dev Performs actions before the delivery of a request. diff --git a/contracts/interfaces/IMech.sol b/contracts/interfaces/IMech.sol index 461f0d5..7ec304a 100644 --- a/contracts/interfaces/IMech.sol +++ b/contracts/interfaces/IMech.sol @@ -19,10 +19,12 @@ interface IMech { function maxDeliveryRate() external returns (uint256); - function getPaymentType() external returns (uint8); + function paymentType() external returns (bytes32); /// @dev Gets finalized delivery rate for a request Id. /// @param requestId Request Id. /// @return Finalized delivery rate. function getFinalizedDeliveryRate(uint256 requestId) external returns (uint256); + + function tokenId() external view returns (uint256); } \ No newline at end of file diff --git a/contracts/interfaces/IMechMarketplace.sol b/contracts/interfaces/IMechMarketplace.sol index 6417713..a027e7b 100644 --- a/contracts/interfaces/IMechMarketplace.sol +++ b/contracts/interfaces/IMechMarketplace.sol @@ -20,12 +20,10 @@ interface IMechMarketplace { /// @dev Delivers a request. /// @param requestId Request id. /// @param requestData Self-descriptive opaque data-blob. - /// @param deliveryMechStakingInstance Delivery mech staking instance address (optional). /// @param deliveryMechServiceId Mech operator service Id. function deliverMarketplace( uint256 requestId, bytes memory requestData, - address deliveryMechStakingInstance, uint256 deliveryMechServiceId ) external;