Description

While the codebase generally follows good coding practices, there are some minor instances where stronger coding conventions could be applied to improve code readability, maintainability, and security.

Impact

Inconsistent coding conventions can diminish code readability and make it harder for third party integrations and future developers to understand the code.

Here are some specific instances:

1. The function VestingFactory.sol -> function createVestingFromNow(address _recipient, uint40 _duration, uint256 _amount, bool _isCancellable) does not have input validation on the duration, allowing for someone to create an irrational duration period. This can lead to some unexpected results; for example, if duration is larger than the amount, the last section of the getAccruedTokens() function in Vesting.sol will always return 0.

2. The following if condition in Spot.sol -> createNewStf(STfSpot calldata _stf) reverts if the token is not a base or deposit token - but uses the NoBaseToken error each time.

if (!isBaseToken[_stf.baseToken] || !isDepositToken[_stf.depositToken]) {
    revert NoBaseToken(_stf.baseToken);
}

3. The function addTokenCapacity(address baseToken, address depositToken, uint96 capacity) in Spot.sol requires the capacity to be nonzero. This means the capacity can never be unset once set.

4. The Trade.sol -> openSpot(...) emits an event emit OpenSpot(spot.managerCurrentStf(msg.sender), ...) at the end. This relays the manager of the stf; however, this information is previously gathered earlier from spot.getManagerCurrentStfInfo(msg.sender). This presents extra, unnecessary gas usage.

5. In Trade.sol -> _closeSpotSwap(), before and after the call to swap.swapUniversalRouter(), the balance of the depositToken is measured and the difference is calculated. Inside Swap.sol -> swapUniversalRouter() the same balance measurement is happening on the receiver parameter, which happens to be depositToken. This difference is returned to closeSpotSwap(). This ends up doing the balance difference calculation twice.

Recommendations

1. Make sure the duration has a sane maximum, preferably less than the amount.

2. Revert with a different reason when the given depositToken is not whitelisted.

3. Consider removing the requirement for capacity being nonzero, unless it is acceptable with no unset functionality.

4. Initialize the OpenSpot event with the existing variable (_stf.manager).

5. If the swapUniversalRouter always returns the same balance change, it can be removed from the closeSpotSwap() function. Otherwise, the measurement can be removed from Swap.

Remediation

STFX acknowledged and resolved the issue in 043fecb3↗, 5fcfa1a4↗, and 0389c664↗.