Spritz.md

Spritz.md

Spritz Finance - Report

19/09/2022

Repository - General Comments

.husky and .yarn I guess you used Paulrberg template. They are quite useless tbh

.vscode Opinionated to keep it in the source repo, especially if not all your devs use VSCode. Better to add a separate document to explain code style (i.e. formatting, line breaks, comment style, NatSpec position, etc.)

hardhat.config.ts Play around with optimiser values (in the range 200-1000), it is often set at 800 but that might not be the best for your codebase. Change the values and run the gas tracer and contract sizer plugins (see below) to properly find the best runs value.

I recommend that you use the following Hardhat plugins too:

• hardhat-spdx-license-identifier: checks that all your files have the appropriate SPDX license line

• hardhat-abi-exporter: to export the abi (you are using typechain but you should also commit the deployed ABI on your repo/documentation website for interoperability with other tools)

• hardhat-gas-reporter: to check the gas costs of each of your smart contract functions and make sure you don't end up with too pricey calls

• hardhat-contract-sizer: to check your smart contracts size (remember the 24Kb limit) and pay special attention to those who surpass 20Kb, especially if you plan to extend/inherit them in the future

• hardhat-tracer: this one is a very nerdy tool but shows you in details all the smart contract calls: extremely useful to see duplicate calls and optimise your code accordingly.

As you increase the number of plugins, to avoid excessively loading the CI pipeline, it is recommended to properly use their own configuration settings. Make use of excludeContracts, only and other settings to only include the relevant files and exclude mocks, tests and interfaces. Likewise, set runOnCompile to false where needed to speed up the CI. You'll mainly use them manually to debug and double check the codebase during the development process.

.solhint.json Why not use this tool to its max and enable all other relevant rules available? Examples are "imports-on-top" or "check-send-result". Then you can disable single lines of code with

// solhint-disable-next-line [rule]

tasks/spritzPay.ts Why don't you have the script store the deployed contracts' addresses in a file? You can then use it as a release.

Codebase - General Comments

1. Why are you not using GitHub or CircleCI actions?

2. You should add a comment at the top of each of your smart contracts to state what it does and how. Make proper use of NatSpec, it will help your future devs and auditors get a glimpse of what a smart contract does before reading it all.

Example:

/// @notice This contract prints "Hello World"
/// @dev You must send 1 ETH or it will revert
/// @link https://yourdocs.com/test
contract Test {

  error stingy_user_detected();

  function print() payable external returns (string memory) {

    if(msg.value < 1e18) revert stingy_user_detected();

    return "Hello World";

  }

}

3. I see a lot of excludes --exclude naming-convention,external-function,low-level-calls in Slither. It is much better not to disable any detector and instead make proper usage of the single line skip comments:

// slither-disable-next-line [detector1,detector2]

Update Slither to the latest version to get the disable comments working!

4. Please check https://twitter.com/openzeppelin/status/1570432196753891328

5. Do you plan to use gasless transactions or GSN? If not _msgSender() is a (almost negligible) waste of gas.

Codebase - Review

Libs

SafeERC20.sol

Duplicate of OpenZeppelin SafeERC20.sol, use theirs:

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

SpritzPayStorage.sol

Custom errors should go in the contract body

contract Test {
  error Custom();
}

When writing events, make sure to properly index relevant params (up to 3 for each event) with the indexed keyword. It will make the frontend task of event log parsing a little bit faster, yet after a certain amount of data volume being generated from your smart contracts you will have to use The Graph or a similar indexing solution.

event Payment(address indexed from, address indexed to);
...
emit(msg.sender, address(this));

WETHUpgradeable.sol

I don't get the rationale behind this contract. WETH is an ERC20, you can use SafeERC20 library to perform most of these menial tasks without the code duplication this library introduces. Moreover, WETH address will not change on any reputable chain.

The only notable difference of WETH compared to other ERC20 is that you can wrap and unwrap ETH, by sending direct ETH to the WETH9 contract you will get back WETH tokens (IWETH(weth).deposit{ value: amount }();), instead by calling withdraw and burning your WETH you'll get back an ETH transfer (IWETH(weth).withdraw(amount);).

You will need to add a special fallback in your smart contract to accept ETH incoming from the WETH contract.

receive() external  payable {
  // decomment it if you want to enable incoming ETH from the WETH withdrawals only
  //if (msg.sender != weth_contract) revert ETH_Callback_Failed();
}

Core

SpritzPayV1.sol As before, custom errors inside the contract. For the Payment event I'd index the to too.

I don't understand the usage of safeTransferToken, just use token.safeTransfer/From, you will get a revert from the SafeERC20 library if anything goes wrong, no need to add your own revert. Same for approveTokenSpend. Therefore, this code may revert

if (paymentToken.allowance(address(this), SPRITZ_PAY_ADDRESS) < amount) {
  paymentToken.safeApprove(SPRITZ_PAY_ADDRESS, 2**256 - 1);
}

because for some ERC20 tokens when the allowance is nonzero you must first set it to zero and then increment it (or use safeIncreaseAllowance directly as safeApprove has been deprecated).

You should add a check and revert on the value sent if dealing with Ethers, much better than getting Uniswap incomprehensible four letter revert errors.

You should keep a list of whitelisted tokens, if not both the source and output at least the destination token. Rather than using bool isNativeSwap = sourceTokenAddress == _wrappedNative; why don't you stick to the standard of using 0xEeeee... as ETH? Doing so it would allow you to catch possible errors, where for example a user sets source and destination token are DAI (which would revert inside Uniswap). Lastly, you are not getting the best rate on most tokens in this way and some swaps may revert due to lack of liquidity or excessive price impact.

This assert should be checked inside Uniswap already require(amounts[1] == paymentTokenAmount, "Swap did not yield declared payment amount");

SpritzSmartPay.sol

Are you sure to have these state variables as immutable?

address internal immutable SPRITZ_PAY_ADDRESS;
address internal immutable AUTO_TASK_BOT_ADDRESS;

You could represent subscriptions as NFTs and use the tokenID as accountancy index. Moreover, you could list the tokens owned by a certain user on-chain using ERC721Enumerable.

tokenAmount seems incorrect. As a stylistic choice I'd cast the token to an IERC20 and use the decimals directly without a static call. Now the paymentAmount should already be formatted from the frontend in the destination token decimals units. If you are using a non-token unit of account (dollars) with 2 decimal points I see the rationale for your choice, but in general it is preferred to stick to a blockchain-native numeraire, be it a stable or ethers.

Security Considerations

Your main security threats fall down into three categories:

• reentrancy with ether transfers (currently commented code)
• swaps
• admin risk

For the first one, as long as you stick to the check-effect-execute pattern you are safe to go, basically remember to update balances before executing the transfer.

Swap risk is mainly making sure to set the slippage correctly to avoid MEV attacks, in your case it is handled by the frontend using Uniswap quoter.

Admin hacks may be very dangerous considering that the governance/owner can directly set state variables like the payment recipient.

Finally, the way you handle approvals and transfers could be simplified.