Review the current Spritz architecture focusing on the design and implementation of the recurrent payments model.
- Identify possible pitfalls in the proposed protocol design
- Suggest a possible roadmap to further decentralise the payment process
- Approve or recommend alternatives to how recurrent payments are being settled and the usage of an external automation bot
- Discuss with the team how the current block fits in the whole protocol architecture
- Repository https://github.com/spritz-finance/contracts-core
- Current protocol design https://whimsical.com/payment-flow-62Mhg18pXp7LygEn5biDVm
Spritz finance is a blockchain-enabled protocol that allows users to manage single and recurring payments from an EOA by settling charges and managing token swaps via a smart contract. The recurrent payment functionality is integrated on the smart contract side by capturing first an approval from the user on the chosen payment token and asynchronously settling charges via pulled transfers. The protocol consists of three parts:
The SmartPay contract takes care of storing the subscriptions’ states and hashes. Users interact with it to add, update or delete records in a CRUD fashion and emit events to allow easier tracking state changes on the off-chain side. It also pulls tokens from the users’ wallet and forwards them to the next contract. A bot constantly checks the contract status and invokes appropriate functions to settle payments.
The SpritzPay contract is responsible for swapping tokens, transferring them to the Spritz wallet and emitting events when the process has been performed.
An external backend watches for incoming transfers to the Spritz wallet, swaps them for fiat money (USD) and sends a bank transfer to the recipient.
The overall architecture seems solid. The logical distinction of a stateful contract to store subscription data and a stateless contract to handle payments could be further improved. In particular.
Have SmartPay just store user data and SpritzPay handle payments only, following the single responsibility principle.
A multi-transfer (user wallet -> SmartPay -> SpritzPay -> Spritz wallet) can be collapsed into a single transfer (user wallet -> Spritz wallet), eventually using a hook on the SpritzPay contract to handle the transferFrom. It also helps security-wise to have all user approvals on a single contract, revoking is way easier.
SpritzPay contract may also implement a market/limit order feature, where if an order is not in token X, then it gets swapped (or a limit order gets placed on the orderbook) for token X. A viable solution would be Cow protocol, that can be implemented on-chain signing swap orders from the smart contract itself; then a settlement bot will pick the order and fill it using a cex, a dex or simply OTC liquidity. Doing so, it will be possible to have a wide range of supported tokens, reduce slippage and mitigate MEV attacks. Downsides are, instead, the asynchronous settlement (swaps may take from a few mins to a few hours to get filled), the gas fees paid on the swapped token and a lesser efficiency having to handle a single token. Eventual leftover “dust” can be either returned to the user or kept by the protocol as extra revenue (1inch does it): if so, a sweep function must be implemented on the SpritzPay contract.
function sweep(IERC20 token, address to) external onlyOwner {
token.transfer(to, token.balanceOf(address(this)));
}A guardian role should be implemented. The guardian can perform watchdog functions, mainly locking or suspending the contract activity, in case an attack or a malicious transaction is detected. Due to the need to react quickly, the choice should fall on a single chosen EOA or 2:3 multisig wallet whereas the governance should adopt a more distributed signature schemes. Only the governance can then re-enable paused/locked smart contract functions. This way, even if the guardian were to be compromised or malicious, their damage would be relatively small, pretty much limited to locking the smart contract activity for a limited period of time. The governance must be able to change the guardian address too.
modifier onlyGuardian {
assert(msg.sender == governance || msg.sender == guardian);
_;
}
function setGuardian(address _guardian) external onlyOwner {
guardian = _guardian;
emit GuardianUpdated(_guardian);
}Using a backed stablecoin (USDC, USDT, EUROC) allows to get lower slippage during crypto-fiat conversion. It is required to sign an agreement with the stablecoin issuer and guarantee a minimum transactional volume. Circle rate, for instance, is at 25bps.
For both consider adding:
-
Multicall as inherited so that your contracts can perform multiple actions combined (for instance, bundle subscription creation and approval). In Solidity, multicalls are used to make multiple function calls in a single transaction. This can be useful for optimizing the performance and gas cost of a smart contract, as it allows for multiple actions to be performed in a single transaction, reducing the number of times the contract must be executed on the blockchain. Multicalls can also be used to perform batch operations on an array of data, or to make multiple reads from a smart contract's storage in a single transaction.
-
Input validation. It is important to weigh the pros and cons of input validation when designing a smart contract and to consider the specific requirements and constraints of the use case. In general, it is a best practice to validate inputs as much as possible, but also keep in mind the trade-offs like increased gas cost and possibility of introducing errors. Since the expected usage is via a frontend, null amount values or invalid token addresses should be filtered before even reaching the smart contract.
uint256 private constant MAX_UINT = 2 ** 256 - 1;Equals
type(uint256).maxerror SpritzPayPaymentFailure();Why emit an error named after another contract
address public immutable ACCEPTED_PAYMENT_TOKEN;Do you really want it to be immutable?
if (paymentToken != ACCEPTED_PAYMENT_TOKEN) revert InvalidPaymentToken();Why have the paymentToken parameter if it is a constant?
uint256 allowance = paymentToken.allowance(address(this), SPRITZ_PAY_ADDRESS);
if (allowance < paymentAmount) {
paymentToken.safeIncreaseAllowance(SPRITZ_PAY_ADDRESS, MAX_UINT - allowance);
}I would simply do paymentToken.approve(SPRITZ_PAY_ADDRESS, type(uint256).max) in the constructor and never care about allowances again. If, in the future, you want to make the paymentToken customisable then you need to add the approvals to each function. Ideally you may want to remove approvals to the old token, so that you can use this function in case the spritz pay wallet gets compromised.
function setPaymentToken(address token) external onlyOwner {
// Remove approvals for the old token
IERC20(token).approve(SPRITZ_PAY_ADDRESS, 0);
token = _token;
// Issue approval for the new token
IERC20(token).approve(SPRITZ_PAY_ADDRESS, type(uint256).max);
}(bool success, bytes memory returndata) = SPRITZ_PAY_ADDRESS.call...Replace with an interface
SpritzPay.payWithTokenSubscription(…);function pause() external onlyOwnerAs previously mentioned, this function should be accessible to Guardians too
function pause() external onlyGuardiansIf you call safeIncreaseAllowance directly with a pre-existing approval value it will fail because approves directly to a new value without going to 0 first. Make sure to call token.approve(addr, 0) first in such cases.
IERC20Upgradeable
Use the parent/least code-heavy interface IERC20 unless you specifically need a child derived functions.
function payWithSwap(
address[] calldata path,
...)Passing an arbitrary path may be an issue, as Uniswap allows multi-routing too.
Let's assume you force input token to be WETH and output to be USDC, an attacker may do:
WETH -> WETH/SCAM -> SCAM/USDC -> USDC
The path is valid but the SCAM token may have a blocked transfer function and take out the entire sum, that gets locked inside the pool.
Try not to use nonReentrant guards and instead think about what is the logical flow and where a reentrancy may happen.
The main issue in the payWithV3Swap function is code complexity, doing too many things all at once
The payWithSwap and payWithV3Swap functions are too complex and do not adhere to the KISS nor the single responsibility principles, doing several things packed in the same function block. Consider using a unique function to settle payments and employ an external swap module instead.
contract SwapModule {
function swap(address from, address to, uint256 amount) external {
// Swap logic
}
}and have the contract import it
ISwapModule public swaphelper;
function setSwapHelper(ISwapModule _swaphelper) external OnlyOwner {
swaphelper = _swaphelper;
emit SwapHelperChanged();
}
function payWithSwap()
...
swaphelper.swap(...);
...Doing so you can easily change swap modules depending on the type of chain the contract is deployed to (Uniswap on Mainnet, Quickswap on Polygon, etc) as well as help introduce any custom logic if in the future the swap method were to be more sophisticated (Cow, orderbook, etc.).
receive() external payable {}Handling ETH/native coin requires extra logic, introduces possible attack vectors (Reentrancy) and complicated the code as it is not an ERC20 compatible token. Is it strictly necessary?
Approvals must be carefully handled. On one hand having the SpritzPay contract only handle transfers will make it easier to avoid hacks, on the other hand infinite approvals are generally a bad practice even from a UX point of view.
Another high level risk that comes intrinsically with swapping tokens is the possibility of a bank run, where the token price drops in value faster than what the market wants to purchase. A limit order may be helpful in this sense compared to a direct instant swap (on Uniswap, for instance) but cannot fully solve the issue. To be investigated further.
It may be possible for a hash collision to occur when converting a bytes32 value to a uint256. This is because the bytes32 data type has a larger range of possible values than the uint256 data type. When a value from the larger range is converted to the smaller range, there is the possibility that multiple input values will map to the same output value, resulting in a collision.
- Forta network: A bot analysing incoming and outgoing transactions in a smart contract to detect possible exploits and frontrun the hacker and/or lock the contract using a Guardian role permission;
- Gelato: The Gelato Network is a decentralised protocol that allows users to automate their interactions with smart contracts by relying on bots. Tasks can be configured from their website or from the smart contract itself. Here is an example:
contract ExampleGelatoExecution is Ownable {
address public immutable weth;
IGelatoOps public immutable ops;
bytes32 public taskId;
event NewGelatoTaskWasAdded(bytes32 indexed taskId);
event GelatoTaskWasRemoved(bytes32 indexed taskId);
event ExecutedByGelato(uint256 gelatoFee);
constructor(address _weth, address _ops) {
weth = _weth;
ops = IGelatoOps(_ops);
}
function createTask() external onlyOwner {
assert(taskId == bytes32(0)); // cannot create if already exitst
bytes memory execData = abi.encodeCall(this.exec, ());
IGelatoOps.ModuleData memory moduleData = IGelatoOps.ModuleData({
modules: new IGelatoOps.Module[](1),
args: new bytes[](1)
});
moduleData.modules[0] = IGelatoOps.Module.RESOLVER;
moduleData.args[0] = abi.encode(address(this), abi.encodeCall(this.check, ()));
taskId = ops.createTask(address(this), execData, moduleData, weth);
emit NewGelatoTaskWasAdded(taskId);
}
function cancelTask() external virtual onlyOwner {
assert(taskId != bytes32(0)); // cannot cancel if not exitst
ops.cancelTask(taskId);
taskId = bytes32(0);
emit GelatoTaskWasRemoved(taskId);
}
function check() external view virtual returns (bool, bytes memory) {
// if returns true the exec function will be executed
}
function exec() external {
// Execute
}
}