Spheron V1

Taking Example of createLease()(same applies for closeAndRenewLease)<br>- Whenever <code>createLease()</code> gets called the function would throw error.<br>- <code>createLease()</code> gets called from <code>Bid.sol::matchOrder()</code>.<br>- In <code>createLease()</code> there is a <code>onlyBidContract()</code> modifier which passes as msg.sender is Bid<br>contract. Inside <code>createLease()</code>,<code>escrow.lockToken()</code> gets called :<br><code>escrow.lockTokens(order.creator, order.token, _toPrecision(leaseCostPlusFee, token.decimals));</code>- <code>lockTokens()</code> contains a <code>onlyRole(DEFAULT_ADMIN_ROLE)</code> which means only admin can call<br>this function but in our case <code>msg.sender</code> would be computeLease contract address so<br><code>lockTokens()</code> function will fail due to the<br><code>onlyRole(DEFAULT_ADMIN_ROLE)</code> modifier.<br>

Some tokens have a fee on transfer, for example USDT. Usually such fee is not enabled but could be re-enabled at any time.<br>With this fee enabled the deposit() and withdraw() function would receive slightly less tokens than the amounts requested in the function parameter. This will create an accounting problem, as the whole <code>amount</code> variable gets added in unlock Tokens mapping but actually it will receive slightly less tokens than <code>amount</code>.<br>So in mapping it should add the actual received amount.<br>As main functionality is dependent on Lock and Unlock Tokens this should be handled correctly

Malicious Admin can steal away provider earnings Whenever provider or user calls <code>closeLease()</code>, malicious admin can frontrun this transaction and increase the <code>protocolPerc</code> to a very high value such that the very major part of tokens as fees.<br>Now as protocolPerc has been set to almost 99% using<code>setProviderFeePercentage()</code> So provider would get very less amount of tokens as earnings and other major amount (99%) goes as fees to protocol.<br>Here providerAmount would be only 1% of total amount and protocolFee would 99% of total amount.<br>

Malicious User would pay less tokens even if he has used the service provider by provider more than the specified time in the order.<br>Lets understand using an example:<br>1. There is a matched order between user and provider and the conditions are `order.numOfBlocks   = 100` and `acceptedPrice = 1e18`<br>2. Let's say after passing of 200 blocks the provider calls `closeLease()` to close the lease.<br>3. As user has used the service for 200 blocks he should pay tokens for 200 blocks.<br>4. But user would only pay the tokens for 100 blocks of service.<br>5. So it would be loss to provider as he will not receive tokens for remaining 100 blocks.<br>6. According to our example loss of 100e18 to provider

There is no check while calling <code>withdrawProviderEarnings()</code> , <code>withdrawFizzNodeEarnings()</code> and <code>with drawProtocolFees</code> that the withdrawls are in pause state or not.If <code>pauseWithdrawal</code> has been set to true there should be no withdrawals from the protocol.But by calling <code>withdrawProviderEarnings()</code> , <code>withdrawFizzNodeEarnings()</code> and <code>withdrawProtocolFees</code> they can withdraw funds even though the <code>pauseWithdrawal</code> has been set to <a href="http://true.it/" target="_blank">true.It</a> breaks the core functionality of the protocol

Provider would not be able to claim earnings if providerRewardWallet is set as contract address.Let's suppose a provider has added himself into registry and has set providerRewardWallet to contract address where he wants the fund.<br>Now when he wants to claim his earnings via Escrow.withdrawProviderEarnings() he would not be able to claim due to the below check <code>require(msg.sender == providerRewardWallet, "Not Provider Reward Wallet");</code> So due to above check the provider would not be able to claim as contract address doesn't have <code>withdrawProviderEarnings</code> function selector and whenever the provider calls <code>withdrawProviderEarn ings()</code> from his address it would revert due to above condition. <code>"Not Provider Reward Wallet"</code>

- Hardcoding any address in contract is bad practice.- If we want to change the address in future we can't change it<br><code>IArbSys public constant ARB_SYS = IArbSys(0x0000000000000000000000000000000000000064);</code><br>Add a variable in constructor to set it and add a setter function whenever you want to change the address

There is an unnecessary check of allowance in deposit() which is not at all required. safeTransferFrom() have a built-in check that the user has provided an allowance or not.<br>So <code>require(erc20.allowance(msg.sender, address(this)) >= _amount, "Insufficient allowance");</code><br>This check is unnecessary and of no use. If there was no allowance check and the user did not approve then at that time also it would revert as the user hasn't provided approval. Because of this reason the check is unnecessary<br>

There are 2 variables which check if it is in a pause state or not.<br>Instead of using 2 variables only 1 variable can be used overall to check if the protocol is in pause state or not.<br>It is preferred to use the Pausable library.

The below two functions are used to do the same thing i.e update the provider registry address, one of these functions are redundant and can be removed