Description

The SatlayerPoolV2::withdraw function does not follow the checks-effects-interactions pattern, and it contains a potential reentrancy issue.

function withdraw(address _token) external {
    if (withdrawAmounts[_token][msg.sender] == 0) revert WithdrawAmountCannotBeZero();
    if (block.timestamp < withdrawTimes[_token][msg.sender]) revert WithdrawAttemptTooEarly(); 
    if (withdrawTimes[_token][msg.sender] == 0) revert WithdrawTimeNotSet();

    IERC20Metadata(_token).safeTransfer(msg.sender, withdrawAmounts[_token][msg.sender]);

    withdrawAmounts[_token][msg.sender] = 0;
    withdrawTimes[_token][msg.sender] = 0;
}

The withdrawAmounts and withdrawTimes mappings for the corresponding token and msg.sender address are updated only after the IERC20Metadata(_token).safeTransfer external call.

Impact

The issue can lead to total training of the pool, should an ERC20Plugins compatbile token be used.

Assuming only legitimate ERC-20 assets are used, and therefore that the attacker cannot gain control during the call to IERC20Metadata(_token).safeTransfer, this issue does not constitute an exploitable vulnerability. As such, the likelihood is low.

However, careful vetting of the allowed ERC-20 assets is required unless a code change is made to mitigate the issue.

Recommendations

We recommend strictly following the checks-effects-interactions pattern as well as updating withdrawAmounts and withdrawTimes before calling IERC20Metadata(_token).safeTransfer.

function withdraw(address _token) external {
    if (withdrawAmounts[_token][msg.sender] == 0) revert WithdrawAmountCannotBeZero();
    if (block.timestamp < withdrawTimes[_token][msg.sender]) revert WithdrawAttemptTooEarly(); 
    if (withdrawTimes[_token][msg.sender] == 0) revert WithdrawTimeNotSet();

-   IERC20Metadata(_token).safeTransfer(msg.sender, withdrawAmounts[_token][msg.sender]);

    withdrawAmounts[_token][msg.sender] = 0;
    withdrawTimes[_token][msg.sender] = 0;

+   IERC20Metadata(_token).safeTransfer(msg.sender, withdrawAmounts[_token][msg.sender]);
}

Additionally, we recommend clearly documenting the types of ERC-20 assets that are allowed to be used with the SatlayerPoolV2 contract.

Remediation

This issue has been acknowledged by SatLayer, and a fix was implemented in commit d1b3111d↗. In addition, the SatLayer team has stated that they do not plan to integrate such tokens, and they will be mindful of this issue in the future.