Grateful A-1

A malicious user can perform a sandwich attack on the liquidate call which results in the liquidate call to be reverted. Consider the following scenario:

1. Attacker creates `giver` profile `p1`
2. Attacker creates `creator` profile `p2`
3. Attacker subscribes `p1` to `p2` with the maximum allowed `rate` value (to make the attack more profitable)
4. At this state, funds are flowing with a high rate from `p1` to `p2`.
5. After some time, `p1` is running out of funds and `canBeLiquidated` returns true.
6. Another user calls `liquidate` on p1, but attacker front-runs the `liquidate` tx:
1. Attacker uses another profile `p3` (or could use p2) and creates subscription to `p1`, thereby setting `inflow > outflow` for p1.
2. Since `inflow > outflow`, the liquidate call initiated by another user will revert.
3. Attacker can back-run the `liquidate` tx and unsubscribes `p3` from `p1`.
7. Funds are still flowing from `p1` to `p2`, thereby increasing the negative balance of `p1` and increasing balance of `p2`.
8. Attacker can withdraw the balance from `p2`; the negative value in p1 remains and must be compensated with funds from the treasury.

Because step 6 happens in the same block, there are no funds flowing from p3 to p1, but the setup is enough to get the liquidate call reverted. Thus, the costs of the attack are only the tx costs for subscribe and unsubscribe.

The attacker can repeatedly perform the attack, to keep the funds flowing from p1 to p2 and thereby draining the funds of the protocol.

Note that this attack only pays off under the following circumstances:

• when the attacker manages to sandwich the liquidate call for the entire liquidation period and beyond.
• when the balance of p2 increased due to preventing liquidation is higher than the cost of the attack (= tx costs for sandwiching liquidate call).

Remediations to Consider

Consider adding a delay between subscribe and unsubscribe to make the above attack vector unprofitable.

When a profile NFT is transferred to a new owner, ProfilesModule.notifyProfileTransfer is called to revoke all existing permissions and to set new owner.

However, for subscription NFTs associated to a profile, the new owner is not set and thus still pointing to the original owner when checking GratefulSubscriptions.ownerOf.

Consider also transferring the profile’s associated subscription NFTs to the new owner when the the profile is transferred to a new owner.

LiquidationModule.liquidate takes liquidatorId as an argument. However, there is no validation done whether the liquidatorId is associated to a profile.

If this is not a requirement, consider including msg.sender when emitting the event.

Also the SubscriptionLiquidated event is emitted with 0 rewards. Since there are no incentives for liquidators in this version of the contract, consider removing the reward argument in the emitted event.

Subscription.getDuration returns the duration + elapsedTime where elapsedTime is calculated as follows:

uint256 elapsedTime = block.timestamp - lastUpdate;

While this is true for active subscriptions, the calculation is not correct for unsubscribed subscriptions, as in this case elapsedTime should be 0.

Some “weird” ERC20 tokens (e.g. USDT) require the approval to be set to 0 before setting it to a new value (see here). For such tokens, VaultsModule.addVault would fail when trying to add a vault implementation that is already used within another vault.

Note that this is seen as a low-level issue, as the scenario of adding multiple vaults pointing to same implementation is considered as very unlikely according to the Grateful team.

VaultUtil.isVaultPaused returns the value from Vault.isPaused(). However, isPaused is not implemented correctly and returns true for unpaused vaults and false for paused vaults.

The only reason why FundsModule.withdrawFunds correctly allows to withdraw funds from unpaused vaults is because it checks against the inverted value as seen below:

if (!VaultUtil.isVaultPaused(vaultId))
   revert VaultErrors.InvalidVault();

Consider changing Vault.isPaused so that it returns true for pause vaults and false for unpaused vaults.

GratefulProfile.tokenURI and GratefulSubscription.tokenURI don’t revert when the token id doesn’t exist but instead returns default values for subscription.

In order to comply with behavior of well known ERC721 implementation like the one from Openzeppelin, consider reverting for non-existent token ids.

Consider removing _EDIT_PERMISSIONS from ProfileRBAC.sol as it is not used anywhere in the code.

In Balance.sol, canWithdraw and canStartSubscription check for both _hasEnoughBalance and _isSolvent:

return _hasEnoughBalance(self, time) && _isSolvent(self, time);

However, _hasEnoughBalance (as it only accounts for outflow) is a much more stricter check, thus making the additional check _isSolvent redundant.

Consider removing _isSolvent from the above functions to save gas costs and make the code more readable.

Note that at the current state, the protocol doesn’t allow burning of GratefulProfile NFTs. If this is something being considered to allow in future versions, it is worth noting that this would break ProfilesModule.createProfile.

The function createProfile calculates the tokenId as follows:

uint256 tokenId = profile.totalSupply() + 1;

When a token id is burned, totalSupply decreases by 1, meaning that for the next call to createProfile, a tokenId will revert, as the token id is created that was already minted before. As a result, safeMint will revert, essentially breaking the entire createProfile functionality.