Tokemak Autopilot Core Contracts Update Security Review Report

Cross-chain messages are sent from EthPerTokenSender to BridgedLSTCalculator on L2 through the MessageProxy whenever there is a change in the rate of an LST on L1. This process is permissionless.

On the L2 side, the BridgedLSTCalculator only checks the timestamp of the received message to be strictly greater than the next message, while EthPerTokenSender allows for sending of multiple messages in the same block and therefore at the same timestamp.

For LST protocols that have permissionless rate updating, it could be possible to have a stale ETH share rate for some LST protocols until their next update period. For example, Stader only does update every 24 hours.

The attacker could initiate the message for the current rate, perform the rate update and then initiate another message. The new rate would be saved in EthPerTokenSender and so another call would revert (since the rate did not change) for 24 hours. However, the second message reverts on the L2 side due to the timestamp check and so the stale rate would be kept for the whole duration.

function _snapshotOnMessageReceive(
    uint256 currentEthPerToken,
    uint256 snapshotTimestamp,
    uint256 newBaseApr
) internal {
    // Message may be retried but if the message is older than one we've already
    // processed we don't want to accept it. The send in the same block on the source chain
    // slither-disable-next-line timestamp
    if (lastBaseAprSnapshotTimestamp >= snapshotTimestamp) {
        revert OnlyNewerValue(lastBaseAprSnapshotTimestamp, snapshotTimestamp);
    }
 
    [..]
 
}

When using cross-chain message we highly recommend to depend on sequence numbers instead of timestamps to preserve chronological order of messages.

In the AerodromeAdapter.sol contract, the IRouter.removeLiquidity() function sets the deadline parameter for liquidity removal to block.timestamp. This can lead to issues because miners have some control over block.timestamp, which may result in time-based manipulation. Furthermore, users are not given the option to specify their own deadline, which is a key feature for ensuring transaction validity within a certain time window:

function _runWithdrawal(AerodromeRemoveLiquidityParams memory _params)
    internal
    returns (uint256[] memory actualAmounts, uint256 lpBurnAmount)
{
    uint256 lpTokensBefore = IERC20(_params.pool).balanceOf(address(this));
    (uint256 amountA, uint256 amountB) = IRouter(_params.router).removeLiquidity(
        _params.tokens[0],
        _params.tokens[1],
        _params.stable,
        _params.maxLpBurnAmount,
        _params.amounts[0],
        _params.amounts[1],
        address(this),
>>      block.timestamp
    );
 
    uint256 lpTokensAfter = IERC20(_params.pool).balanceOf(address(this));
 
    lpBurnAmount = lpTokensBefore - lpTokensAfter;
 
    actualAmounts = new uint256[](2);
    actualAmounts[0] = amountA;
    actualAmounts[1] = amountB;
}

In contrast, the MaverickAdapter.sol contract handles this situation correctly by allowing the user to pass a deadline as a parameter in the MaverickWithdrawalExtraParams struct. This provides users with greater flexibility and protection against potential vulnerabilities:

function _runWithdrawal(
    IRouter router,
    uint256[] calldata amounts,
    MaverickWithdrawalExtraParams memory maverickExtraParams
) private returns (uint256 tokenAAmount, uint256 tokenBAmount, IPool.BinDelta[] memory binDeltas) {
    (tokenAAmount, tokenBAmount, binDeltas) = router.removeLiquidity(
        IPool(maverickExtraParams.poolAddress),
        address(this),
        maverickExtraParams.tokenId,
        maverickExtraParams.maverickParams,
        amounts[0],
        amounts[1],
        maverickExtraParams.deadline
    );
}

The deadline parameter should be implemented so that users can specify their desired deadline.

In the function AutoPoolETHStrategyConfig::_validateHooks(), the variable zeroAddressReached is initialized with the value false. However, the default value of a new boolean variable is always false, making this initialization redundant.

bool zeroAddressReached = false;

There is no need to initialize zeroAddressReached with the value false

- bool zeroAddressReached = false;
+ bool zeroAddressReached;

The PRICE_STALE_CHECK parameter in the AerodromeStakingIncentiveCalculator.sol contract is currently hardcoded to a fixed value of 12 hours. This design choice may hinder the protocol's ability to react to volatile market conditions effectively.

/// @dev Duration after which a price/data becomes stale.
uint40 public constant PRICE_STALE_CHECK = 12 hours;

As market dynamics change, especially in volatile DeFi ecosystems, the rigidity of this fixed parameter may lead to inaccurate incentive pricing, liquidity provider token prices, and staking APR calculations. Consequently, outdated price data could be used during key contract operations, potentially leading to suboptimal reward distributions or incorrect incentive calculations.

function _getIncentivePrice(address _token) internal view returns (uint256) {
    IIncentivesPricingStats pricingStats = systemRegistry.incentivePricing();
    (uint256 fastPrice, uint256 slowPrice) = pricingStats.getPrice(_token, PRICE_STALE_CHECK);
    return Math.min(fastPrice, slowPrice);
}

Consider implementing a setter function for the PRICE_STALE_CHECK parameter. This function should only be callable by a trusted entity (such as the owner or a governance contract) to prevent abuse.

In the project, there are several scripts incorrectly named with the .sol extension instead of the expected .s.sol suffix. This inconsistency can cause confusion and prevent Forge from recognizing these files as scripts, potentially leading to build or execution issues.

script/base/02_DestinationTemplatesSetup.sol
script/base/06_StatBridging.sol
script/base/07_StatBridgingSet2.sol
script/mainnet/02_DestinationTemplatesSetup.sol
script/mainnet/10_Maverick.sol
script/mainnet/11_StatBridging.sol
script/mainnet/12_StatBridgingBaseReceipt.sol
script/mainnet/13_StatBridgingBaseSet2.sol
script/mainnet/14_ChainlinkPerTokenSender.sol
script/sepolia/04_Router.sol
script/utils/Constants.sol

All script files should follow the .s.sol naming convention to align with Foundry's standards and ensure clarity between contracts, libraries, and scripts.

The current implementation of the calculateEthPerToken() function includes a conditional check for a totalTVL value of zero, which returns 0 when detected:

if (totalTVL == 0) {
    return 0;
}

However, the likelihood of totalTVL being zero is extremely low due to the constant accumulation of staked assets in the protocol. If totalTVL were zero, the existing calculation logic would naturally return 0 in the final division step, making this explicit check redundant.

function calculateEthPerToken() public view override returns (uint256) {
    // Get the total TVL priced in ETH from restakeManager
    // slither-disable-next-line unused-return
    (,, uint256 totalTVL) = renzoRestakeManger.calculateTVLs();
 
    uint256 totalSupply = IERC20(lstTokenAddress).totalSupply();
 
    if (totalSupply == 0) {
        return 1e18;
    }
 
    if (totalTVL == 0) {
        return 0;
    }
 
    return (10 ** 18 * totalTVL) / totalSupply;
}

Remove the redundant check:

function calculateEthPerToken() public view override returns (uint256) {
    // Get the total TVL priced in ETH from restakeManager
    // slither-disable-next-line unused-return
    (,, uint256 totalTVL) = renzoRestakeManger.calculateTVLs();
 
    uint256 totalSupply = IERC20(lstTokenAddress).totalSupply();
 
    if (totalSupply == 0) {
        return 1e18;
    }
 
    return (10 ** 18 * totalTVL) / totalSupply;
}
 
-- if (totalTVL == 0) {
--     return 0;
-- }