We have decided to use the AGPLv3 license for our codebase. We need to specify the SPDX license identifier in every file to be AGPLv3. Also, include the license notice for AGPLv3

Description:
All of the for-loops in the project are using the standard way of declaring them, which is slightly inefficient gas-wise.

The following loops are not optimized:

• StrollManager.sol - Line 116

Solution:
Consider optimizing the for-loops in each contract to be more efficient in terms of its
gas usage, including:

• Not initializing variables with their default value
• Using the more efficient way to increment a variable

Location: strategies/AaveV2StrollOut.sol - Line 86 - 87

Description:
In the topUp function, supertokens are transferred to the user without making use of the SafeERC20 variant even though it’s imported and used for other transfers. This might be the case as superTokens can be trusted with their implementation being very familiar with the developers, but it would still be consistent to use SafeERC20 for any transfers. The same also applies to strategies/ERC20StrollOut.sol in lines 62 - 63.

Justification:
Forgot the fact that a supertoken inherits IERC20.

Solution:
Will change the code to make it consistent with other types of transfers.

Add the Gelato keeper contract code in the repo.

Location: StrollManager.sol - Line 172

Description:
As topUp.superToken is already of type ISuperToken and not an address, the cast is unnecessary.

Justification:
Slipped by us.

Solution:
Consider not unnecessarily casting topUp.superToken to ISuperToken.

Location: StrollManager.sol - Line 69 - 70

Description:
The index is the keccak256 hash of the users' address, the superToken address, and the liquidityToken address. Subsequently, the index is used to store the top-up of the user. In order not to overwrite anything, it is important to check whether there is already a top-up stored for the user, which is also indicated by the comment. However, this check is currently not implemented.

Justification:
The purpose of not having a check in the first place was to allow for top-up updates to happen using this same function (createTopUp). However, it makes more sense to separate these two functionalities as this would also allow us to save some gas costs.

For upgrading ERC20 tokens to supertokens, we require providing allowance to the supertoken contract. This is done in individual strategy contracts. However, there is a chance that we might hit uint256 max amount (very unlikely but possible).

Suggest to check for allowance and also check if the top up is possible with the current allowance to the supertoken contract. If not, approve the contract once again.

The code for the same should ideally be in StrollManager.sol. However, to not break existing deployments, it's recommended to not change it from strategy contracts. However for the next version, see to it that it's moved. This will also help us with gas savings.

Location: StrollManager.sol - Line 27 - 35

Description:
There is no logic in the contract that actually enforces minLower to be less than minUpper and vice versa. As there is no way to change the values of minLower and minUpper, it should be ensured that they are correct. Additionally, if minLower is set to 0, this would allow for unnecessary 0 TopUps by the Keepers to potentially happen.

Justification:
Just wanted to save some gas by avoiding trivial checks. However, a good point has been raised as we need a way to change minUpper and minLower parameters.

Solution:
Add some trivial checks but more importantly, add functions to change minLower and minUpper limits. It's worth noting that existing top-ups whose limits may not adhere changed global limits i.e., minLower > userTopUpLowerLimit or minUpper > userTopUpUpperLimit; we need to enforce the global limits on them as well.

topUps is a private struct that could have been public. Because of this, there are two extra functions (getTopUpByIndex and getTopUp). Change the access modifier to public to reduce contract size.

We are getting to the point that documentation will be needed to Stroller Protocol.

Any opinions on this? Was thinking on using gitbook to create a doc based website to the protocol and "how to".

Natsoec comments are missing from StrollManager.sol. Add them in the interfaces of the implementation contracts.

Location: strategies/StrategyBase.sol - General

Description:
In the StrategyBase contract, there is a changeStrollManager() function, which is seemingly also used to initialize the value of the strollManager variable. According to the best practice, this should be done via the constructor.

Justification:
We included this function to allow for changing strollManager in case we discover any bugs in strollManager.

Solution:
None.

We need deployment scripts to allow for contract deployments on different chains quickly. These scripts already exist but are outdated and need modification.

Currently, the contracts don't give any reason as to why a top-up isn't necessary. However, we need the reason for monitoring purposes. Change the checkTopUpByIndex function to return the reason code.

Location: strategies/StrategyBase.sol - Line 31 - 32

Description:
In the emergencyWithdraw() function, arbitrary tokens can be transferred out of the contract in case they got stuck, etc. As errors can also occur in the ERC20 contract itself during the transfer() call, the custom TransferFailed error may not be emitted accordingly.

As an additional remark, using safeTransfer instead of
transfer() would make a lot of sense here, as downstream contracts use it anyways, and subsequently not using it would not save any gas cost otherwise.

Justification:
Slipped by us.

Solution:
Will implement the recommended solution of making use of a try-catch-block to ensure that the custom TransferFailed error is thrown each time a transfer failed due to any reason.

Also will use safeTransfer instead of a regular transfer.

Description:
In certain circumstances, it saves gas to cache variables that are read often, especially if they are stored in the storage and not in the memory.

The affected variables are:

• StrollManager.sol - Line 116 (_indices.length)

Solution:
Consider caching the affected variables and reading from the cached version instead of doing costly read actions.

Location: StrollManager.sol - Line 165 - 167

Description:
The checkTopUpByIndex function determines the required top-up amount, which returns zero in cases where e.g. the user's balance or allowance doesn’t suffice. Additionally, however, the case of a supertoken being disabled for the corresponding strategy is handled in the same if-clause and returns the same zero output. Subsequently, it also triggers the TopUpNotRequired error, which in this case is not correct, as there might very well be a top-up required.

Justification:
Never really thought about showing the reason why performing a top-up may fail.

Solution:
As recommended, the checkTopUpByIndex function will return the error along with the amount so, in case of the amount being 0, an error is checked for.

Location: strategies/AaveV2StrollOut.sol - Line 43

Description:
Generally, it is advisable to properly verify return values early, especially if they are used for subsequent calls without a potential error quickly appearing. In this case, an underlying token with the zero address in line 43 would lead to a lot of unnecessary calls and computations that could have been prevented. The same also applies to strategies/ERC20StrollOut.sol in line 29.

Justification:
We are already checking if the underlying token is correct (non-zero) or not when creating a top-up. Unless a supertoken can change its underlying token, this issue isn't a problem at all.

Location: StrollManager.sol - Line 122 - 131

Description:
Similar to the way the removeApprovedStrategy() function works, it is a common best practice to only emit events in cases where the state actually changed. This allows external tools to reconstruct the current state and all of the state changes without diving into the code or any specific transactions. The same also applies to strategies/StrategyBase.sol in lines 15 - 25.

Justification:
Slipped by us.

Solution:
Consider changing the function to only emit the event if the strategy wasn’t approved already, e.g. via something like this
if (approvedStrategies[strategy]) return;
or revert the transaction in case the strategy is already approved.

This project is small enough to learn and take advantage of Foundry test suite. Once the contracts have stabilised and sent for audit, process of migration can begin.

Currently, the contracts don't allow for partial amount top-ups. For example, if the supertoken amount to be topped up is worth that of 8 days of liquidity but the user only has the liquidity tokens worth 5 days of supertoken amount, the contract/strategy should still top up using all the available liquidity.