Velodrome

Description

The AutoCompounder.swapTokenToVELOWithOptionalRoute() and AutoConverter.swapTokenToTokenWithOptionalRoute() functions include time restrictions that prevent keepers from calling them before the second hour of an epoch begins.

However, the Swapper contract lacks this time restriction, allowing keepers to call the swapToTargetToken() function at any time during an epoch. To maintain consistency between contracts, this time restriction should also be implemented in the Swapper contract.

Recommendation

Add the same time restriction validation to the swapToTargetToken() function to ensure consistency with the relays.

Velodrome: This issue has been addressed in commits: - SC change + unit tests: 97371d2 - Fuzz tests: c0ffee5

Cantina Managed: Verified fixes.

Description

The Swapper contract uses the standard Ownable implementation from OpenZeppelin, which allows the owner to transfer ownership directly to a new address in a single transaction. This introduces a risk where the ownership of the contract could be accidentally transferred to an incorrect address.

The Ownable2Step pattern provides a more secure ownership transfer mechanism that requires a two-step process whereby the owner initiates a transfer, and the new owner must accept it. This prevents accidental transfers to incorrect or inaccessible addresses.

Recommendation

Replace the current Ownable implementation with Ownable2Step :

- import "@openzeppelin/contracts/access/Ownable.sol";+ import "@openzeppelin/contracts/access/Ownable2Step.sol";- contract Swapper is Ownable {+ contract Swapper is Ownable2Step {  // ...}

Velodrome: The finding has been acknowledged and will not be fixed. The owner of the Swapper contract will be verified prior to granting the Swapper permissions required for up-keeping operations.

Cantina Managed: Acknowledged.

Description

The swapToTargetToken function accepts arbitrary _commands and _inputs parameters set by the Keeper, who has a trusted role. While the Keeper is responsible for providing correct values to ensure proper swap functionality, the code lacks to document that these parameters are arbitrary but trusted by the Keeper.

Recommendation

Add explicit documentation or comments clarifying that _commands and _inputs parameters are provided by the trusted Keeper role. This comment prevents any potential misunderstanding of the input parameters.

Velodrome: Addressed in commit c407268.

Cantina Managed: Verified fix.

Description

The Swapper contract uses the keeper role to facilitate swaps to the relay's target token, using Uniswap's universal router interface to do this. This keeper role is trusted and operated initially by the Velodrome team. To avoid significant reliance on this role, including an upper bound on slippage can help protect relay contracts from being intentionally sandwiched due to compromise.

Recommendation

Ensure it is intended for the keeper role to not have any designated max slippage as per the AutoCompounder and AutoConverter implementations.

Velodrome: The finding has been acknowledged and will not be fixed. The Keeper infrastructure will be managed internally and slippage will be handled off-chain, to prevent excessive slippage while avoiding additional oracle checks on-chain.

Cantina Managed: Acknowledged.

Description

The Swapper contract relies on a trusted keeper role to facilitate swaps from arbitrary tokens held by the relay to some target token. While most input variables are checked, the keeper can choose which slippage to set and therefore is not enforced to optimally route the swap. In terms of design, there could be considerable improvement in efficiency and dependency on trusted roles by moving to an dutch auction style token sale.

By taking this path, at a given interval, the dutch auction will find the optimal conversion rate for which _inputToken -> _targetToken without needing to manage slippage controls on-chain. There is one caveat in the fact that this does introduce latency but because rewards and other accruals are usually done on an epoch basis within Velodrome, this is not overtly concerning.

Recommendation

Consider making efforts to further decentralize the Swapper contract in order to remove it's dependency on permissioned roles.

Velodrome: The finding has been acknowledged and will not be fixed. The Keeper infrastructure for this project will be managed internally, so the same Keeper flow will be preserved in the Swapper and Relays for consistency.

Cantina Managed: Acknowledged.

Description

The Swapper contract calculates the beginning of an epoch using:

uint256 _epoch = block.timestamp - (block.timestamp % WEEK);

This calculation can be simplified by using the VelodromeTimeLibrary.epochStart() function.

Recommendation

Replace the direct calculation with the library function:

- uint256 private constant WEEK = 7 days;...
function swapToTargetToken(...){	...	if (!_isAutoCompounder) {-    uint256 _epoch = block.timestamp - (block.timestamp % WEEK);-    amountTokenEarned[_relay][_epoch] += _amountOut;+    amountTokenEarned[_relay][VelodromeTimeLibrary.epochStart(block.timestamp)] += _amountOut;	}	...}

Velodrome: The issue has been acknowledged and will not be fixed due to a mismatch in Solidity versions. The Swapper-related contracts are using a newer Solidity version than the library, so the epoch calculation logic was replicated in the Swapper contract to avoid version downgrades.

Cantina Managed: Acknowledged.