Farseen0/scar-pairs huggingface dataset:
LOW severity: [LID-18] Finalisation share rate sanity check does not take postponed share burning into account. **Severity:** Medium

**Path:** Lido.sol:_handleOracleReport#L1187-L1298

**Description:**

When an oracle report with a withdrawal request finalisation comes in, the corresponding ETH is sent to the Withdrawal Queue contract and the shares are burned through the Burner contract. The Burner...

Vulnerable code:
function _checkSimulatedShareRate(
        LimitsList memory _limitsList,
        uint256 _noWithdrawalsPostTotalPooledEther,
        uint256 _noWithdrawalsPostTotalShares,
        uint256 _simulatedShareRate
    ) internal pure {
        uint256 actualShareRate = (
            _noWithdrawalsPostTotalPooledEther * SHARE_RATE_PRECISION_E27
        ) / _noWithdrawalsPostTotalShares;

        if (actualShareRate == 0) {
            // can't finalize anything if the actual share rate is zero
            revert IncorrectSimulatedShareRate(MAX_BASIS_POINTS);
        }

        uint256 simulatedShareDiff = Math256.abs(
            SafeCast.toInt256(_simulatedShareRate) - SafeCast.toInt256(actualShareRate)
        );
        uint256 simulatedShareDeviation = (MAX_BASIS_POINTS * simulatedShareDiff) / actualShareRate;

        if (simulatedShareDeviation > _limitsList.simulatedShareRateDeviationBPLimit) {
            revert IncorrectSimulatedShareRate(simulatedShareDeviation);
        }
    }

burntCurrentWithdrawalShares = Math.min(_sharesToBurnFromWithdrawalQueue, sharesCommittedToBurnNow);
LOW severity: [LID-3] Gas optimisation of canDeposit. **Severity:** Low

**Path:** DepositSecurityModule.sol:canDeposit, Lido.sol:canDeposit#L385-L395, #L672-L674

**Description:**

The function `canDeposit` in both contracts checks whether it is currently allowed to deposit to the deposit contract. Both checks contain conditionals over multiple...

Vulnerable code:
function canDeposit(uint256 stakingModuleId) external view validStakingModuleId(stakingModuleId) returns (bool) {
    bool isModuleActive = STAKING_ROUTER.getStakingModuleIsActive(stakingModuleId);
    uint256 lastDepositBlock = STAKING_ROUTER.getStakingModuleLastDepositBlock(stakingModuleId);
    bool isLidoCanDeposit = LIDO.canDeposit();
    return (
        isModuleActive
        && quorum > 0
        && block.number - lastDepositBlock >= minDepositBlockDistance
        && isLidoCanDeposit
    );
}

function canDeposit() public view returns (bool) {
    return !IWithdrawalQueue(getLidoLocator().withdrawalQueue()).isBunkerModeActive() && !isStopped();
}

function canDeposit(uint256 stakingModuleId) external view validStakingModuleId(stakingModuleId) returns (bool) {
    return (
        quorum > 0
        && STAKING_ROUTER.getStakingModuleIsActive(stakingModuleId)
        && block.number - STAKING_ROUTER.getStakingModuleLastDepositBlock(stakingModuleId) >= minDepositBlockDistance
        && LIDO.canDeposit()
    );
}
LOW severity: [LID-4] DepositSecurityModule redundant staking module ID checks. **Severity:** Low

**Path:** DepositSecurityModule.sol:pauseDeposits, unpauseDeposits, canDeposit, depositBufferedEther (L336-365, L372-378, L385-395, L413-439)

**Description:**

The modifier `validStakingModuleId` in `DepositSecurityModule.sol` is redundant. Each of the functions that make use...

Vulnerable code:
modifier validStakingModuleId(uint256 _stakingModuleId) {
    if (_stakingModuleId > type(uint24).max) revert StakingModuleIdTooLarge();
    _;
}
LOW severity: [LID-8] Gas optimisation of withdrawal queue initialisation. **Severity:** Low

**Path:** WithdrawalQueueBase.sol:_initializeQueue#L470-L476

**Description:** 

The function `_initializeQueue` is called upon initialisation of the Withdrawal Queue contract. We found that it could be further gas optimised.

1. A zero struct `WithdrawalRequest` is created and...

Vulnerable code:
_getQueue()[0].timestamp = uint64(block.timestamp);
_getQueue()[0].claimed = true;

function _initializeQueue() internal {
    // setting dummy zero structs in checkpoints and queue beginning
    // to avoid uint underflows and related if-branches
    // 0-index is reserved as 'not_found' response in the interface everywhere
    _getQueue()[0] = WithdrawalRequest(0, 0, payable(0), uint64(block.number), true);
    _getCheckpoints()[getLastCheckpointIndex()] = DiscountCheckpoint(0, 0);
}
HIGH severity: [LID-9] WithdrawalRequestERbalance does not decrease after claiming. **Severity:** Low

**Path:** WithdrawalRequestERC721.sol:balanceOf#L112-L115

**Description:**

A withdrawal request implements ERC721 and so acts as an NFT. Once a withdrawal request is claimed, it cannot be transferred anymore and NFT should have been burned.

This is also apparent from...

Vulnerable code:
function balanceOf(address _owner) external view override returns (uint256) {
    if (_owner == address(0)) revert InvalidOwnerAddress(_owner);
    return _getRequestsByOwner()[_owner].length();
}
LOW severity: [LID-16] Adding a node operator does not increase the nonce. **Severity:** Low

**Path:** NodeOperatorRegistry.sol:addNodeOperator#L302-L322

**Description:**

The function to add a new node operator to the node operator registry does not increase the staking module nonce, even though this nonce should increase upon the activation of a node operator...

Vulnerable code:
function addNodeOperator(string _name, address _rewardAddress) external returns (uint256 id) {
    _onlyValidNodeOperatorName(_name);
    _onlyNonZeroAddress(_rewardAddress);
    _auth(MANAGE_NODE_OPERATOR_ROLE);

    id = getNodeOperatorsCount();
    require(id < MAX_NODE_OPERATORS_COUNT, "MAX_OPERATORS_COUNT_EXCEEDED");

    TOTAL_OPERATORS_COUNT_POSITION.setStorageUint256(id + 1);

    NodeOperator storage operator = _nodeOperators[id];

    uint256 activeOperatorsCount = getActiveNodeOperatorsCount();
    ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount + 1);

    operator.active = true;
    operator.name = _name;
    operator.rewardAddress = _rewardAddress;

    emit NodeOperatorAdded(id, _name, _rewardAddress, 0);
}
LOW severity: [LID-10] Staking Router redundant module ID checks. **Severity:** Low

**Path:** StakingRouter.sol:getStakingModuleIsStopped, getStakingModuleIsDepositsPaused, getStakingModuleIsActive (L781-786, L788-793, L795-800)

**Description:**

The modifier `validStakingModuleId` checks whether the `_stakingModuleId` is a valid ID. However, the modifier is...

Vulnerable code:
function getStakingModuleIsStopped(uint256 _stakingModuleId) external view
    validStakingModuleId(_stakingModuleId)
    returns (bool)
{
    return getStakingModuleStatus(_stakingModuleId) == StakingModuleStatus.Stopped;
}

function getStakingModuleIsDepositsPaused(uint256 _stakingModuleId) external view
    validStakingModuleId(_stakingModuleId)
    returns (bool)
{
    return getStakingModuleStatus(_stakingModuleId) == StakingModuleStatus.DepositsPaused;
}

function getStakingModuleIsActive(uint256 _stakingModuleId) external view
    validStakingModuleId(_stakingModuleId)
    returns (bool)
{
    return getStakingModuleStatus(_stakingModuleId) == StakingModuleStatus.Active;
}
LOW severity: [LID-14] Redundant storage write. **Severity:** Low

**Path:** StakingRouter.sol:updateExitedValidatorsCountByStakingModule#L271-L303

**Description:**

The function updates the exited validator count for a staking module. The function only checks whether the reported value is less than the previous value and if so...

Vulnerable code:
function updateExitedValidatorsCountByStakingModule(
        uint256[] calldata _stakingModuleIds,
        uint256[] calldata _exitedValidatorsCounts
    )
        external
        onlyRole(REPORT_EXITED_VALIDATORS_ROLE)
    {
        for (uint256 i = 0; i < _stakingModuleIds.length; ) {
            StakingModule storage stakingModule = _getStakingModuleById(_stakingModuleIds[i]);
            uint256 prevReportedExitedValidatorsCount = stakingModule.exitedValidatorsCount;
            if (_exitedValidatorsCounts[i] < prevReportedExitedValidatorsCount) {
                revert ErrorExitedValidatorsCountCannotDecrease();
            }

            (
                uint256 totalExitedValidatorsCount,
                /* uint256 totalDepositedValidators */,
                /* uint256 depositableValidatorsCount */
            ) = IStakingModule(stakingModule.stakingModuleAddress).getStakingModuleSummary();

            if (totalExitedValidatorsCount < prevReportedExitedValidatorsCount) {
                // not all of the exited validators were async reported to the module
                emit StakingModuleExitedValidatorsIncompleteReporting(
                    stakingModule.id,
                    prevReportedExitedValidatorsCount - totalExitedValidatorsCount
                );
            }
            stakingModule.exitedValidatorsCount = _exitedValidatorsCounts[i];
            unchecked { ++i; }
        }
    }
LOW severity: [LID-22] Rebase Limiter isLimitReached will almost always return false due to rounding error. **Severity:** Low

**Path:** PositiveTokenRebaseLimiter.sol:isLimitReached#L103-L115

**Description:**

The function `isLimitReached` is used in `getSharesToBurnLimit` to provide an early return if the rebase limit is already reached and it can just return 0. However, we found that this function...

Vulnerable code:
function isLimitReached(TokenRebaseLimiterData memory _limiterState) internal pure returns (bool) {
    if (_limiterState.positiveRebaseLimit == UNLIMITED_REBASE) return false;
    if (_limiterState.currentTotalPooledEther < _limiterState.preTotalPooledEther) return false;

    uint256 accumulatedEther = _limiterState.currentTotalPooledEther - _limiterState.preTotalPooledEther;
    uint256 accumulatedRebase;

    if (_limiterState.preTotalPooledEther > 0) {
        accumulatedRebase = accumulatedEther * LIMITER_PRECISION_BASE / _limiterState.preTotalPooledEther;
    }

    return accumulatedRebase >= _limiterState.positiveRebaseLimit;
}
LOW severity: [LID-23] Rebase Limiter maxTotalPooledEther optimisation. **Severity:** Low

**Path:** PositiveTokenRebaseLimier.sol:increaseEther#L139-L160

**Description:**

The calculation for `maxTotalPooledEther` in `increaseEther` depends fully on two variables that are only set during initialisation and won't change any further. The calculation can therefore be...

Vulnerable code:
function increaseEther(
    TokenRebaseLimiterData memory _limiterState, uint256 _etherAmount
)
    internal
    pure
    returns (uint256 consumedEther)
{
    if (_limiterState.positiveRebaseLimit == UNLIMITED_REBASE) return _etherAmount;

    uint256 prevPooledEther = _limiterState.currentTotalPooledEther;
    _limiterState.currentTotalPooledEther += _etherAmount;

    uint256 maxTotalPooledEther = _limiterState.preTotalPooledEther +
        (_limiterState.positiveRebaseLimit * _limiterState.preTotalPooledEther) / LIMITER_PRECISION_BASE;

    _limiterState.currentTotalPooledEther
        = Math256.min(_limiterState.currentTotalPooledEther, maxTotalPooledEther);

    assert(_limiterState.currentTotalPooledEther >= prevPooledEther);

    return _limiterState.currentTotalPooledEther - prevPooledEther;
}
HIGH severity: [LID-24] Withdrawal Queue ERownerOf gas optimisation. **Severity:** Low

**Path:** WithdrawalQueueERC721.sol:ownerOf#L156-L163

**Description:**

In the `ownerOf` function, the `WithdrawalRequest` of the corresponding `requestId` is retrieved. The request is loaded fully into memory and both storage slots will be loaded.

However, only the values...

Vulnerable code:
function ownerOf(uint256 _requestId) public view override returns (address) {
    if (_requestId == 0 || _requestId > getLastRequestId()) revert InvalidRequestId(_requestId);

    WithdrawalRequest memory request = _getQueue()[_requestId];
    if (request.claimed) revert RequestAlreadyClaimed(_requestId);

    return request.owner;
}
HIGH severity: [LID-25] Anyone can always transfer 1 wei stETH. **Severity:** Low

**Path:** StETH.sol:getSharesByPooledEth#L311-L315

**Description:**

The functions getSharesByPooledEth and getPooledEthByShares are used to convert from stETH to shares and vice versa. Normally, the pooled ETH amount is greater than the number of shares.

Both functions round...

Vulnerable code:
function getSharesByPooledEth(uint256 _ethAmount) public view returns (uint256) {
    return _ethAmount
        .mul(_getTotalShares())
        .div(_getTotalPooledEther());
}
function _transfer(address _sender, address _recipient, uint256 _amount) internal {
    uint256 _sharesToTransfer = getSharesByPooledEth(_amount);
    _transferShares(_sender, _recipient, _sharesToTransfer);
    _emitTransferEvents(_sender, _recipient, _amount, _sharesToTransfer);
}
function _emitTransferEvents(address _from, address _to, uint _tokenAmount, uint256 _sharesAmount) internal {
    emit Transfer(_from, _to, _tokenAmount);
    emit TransferShares(_from, _to, _sharesAmount);
}

function _transfer(address _sender, address _recipient, uint256 _amount) internal {
    uint256 _sharesToTransfer = getSharesByPooledEth(_amount);
    if (_sharesToTransfer == 0)
        return;
    _transferShares(_sender, _recipient, _sharesToTransfer);
    _emitTransferEvents(_sender, _recipient, _amount, _sharesToTransfer);
}
HIGH severity: [ASTRO-13] Wrong debt calculations during withdrawal. **Severity:** Critical

**Path:** Crate.sol

**Description:** The functions `safeWithdraw`, and `safeRedeem` have inconsistent debt calculation logic.

A bad actor can call `safeWithdraw/safeRedeem` function with such parameters that revert `swapVirtualToAsset` function call (for example, with a...

Vulnerable code:
uint256 _amount,
uint256 _shares,
uint256 _minAmountOut,
uint256 _deadline,
address _receiver,
address _owner
) internal nonReentrant returns (uint256 recovered) {
if (_amount == 0 || _shares == 0) revert AmountZero();

if (liquidityPoolEnabled) {
// We don't take into account the eventual slippage, since it will
// be paid to the depositoors
liquidityPool.debt -= Math.min(_amount, liquidityPool.debt);
try
liquidityPool.swap.swapVirtualToAsset(
_amount,
_minAmountOut,
_deadline,
_receiver
)
returns (uint256 dy) {
recovered = dy;
} catch {
// if the swap fails, we send the funds available
asset.safeTransfer(_receiver, _amount);
recovered = _amount;
}
} else {
// If the liquidity pool is not enabled, we send the funds available
// This allows for the bootstrapping of the pool at start
asset.safeTransfer(_receiver, _amount);
recovered = _amount;
}

emit Withdraw(msg.sender, _receiver, _owner, _amount, _shares);
return (recovered);
}
HIGH severity: [ASTRO-22] No slippage protection for bridgeFunds. **Severity:** High

**Path:** BridgeConnectorHomeSTG.sol, BridgeConnectorRemoteSTG.sol

**Description:** The function bridgeFunds (in both Home and Remote contracts) is used to send assets from a crate to a remote chain; it uses Stargate to make the swap. Although the minimal output amount...

Vulnerable code:
function bridgeFunds(
        uint256 _amount,
        uint256 _chainId
    ) external payable override onlyCrate {B
        // Loading this in memory for gas savings
        // We send directly to the allocator
        address destination = allocatorsMap[_chainId];
        uint256 dstPoolId = dstPoolIdMap[_chainId];
        if (dstPoolId == 0) {
            revert PoolNotSet(_chainId);
        }

        // Bridging using Stargate
        IStargateRouter(stgEndpoint).swap{ value: msg.value }(
            lzChainIdMap[_chainId], // destination chain Id
            srcPoolId, // local pool Id (ex: USDC is 1)
            dstPoolIdMap[_chainId], // remote pool Id
            payable(tx.origin), // refund address for extra gas
            _amount, // quantity to swap
            0, // the min qty you would accept on the destination  
            IStargateRouter.lzTxObj(0, 0, bytes("")), // params for gas forwarding
            abi.encodePacked(destination), // receiver of the tokens
            bytes("") // data for the destination router
        );
        emit BridgeSuccess(_amount, _chainId, msg.value, destination);
    }
MEDIUM severity: [ASTRO-8] Wrong Emergency Stop pattern usage. **Severity:** Medium

**Path:** Crate.sol

**Description:** The contract  `Crate.sol` provides an Emergency Stop pattern, but some functions don’t have any checks to halt their usage, so users can withdraw or redeem after the owner has paused the contract. 

**Remediation:**  Use `whenNotPaused`...

Vulnerable code:
function safeWithdraw(
        uint256 _amount,
        uint256 _minAmount,
        uint256 _deadline,
        address _receiver,
        address _owner
    ) external returns (uint256 shares) {
        // This represents the amount of crTokens that we're about to burn
        shares = convertToShares(_amount);
        _withdraw(_amount, shares, _minAmount, _deadline, _receiver, _owner);
    }

function redeem(
        uint256 _shares,
        address _receiver,
        address _owner
    ) external returns (uint256 assets) {
        return (
            _withdraw(
                convertToAssets(_shares),
                _shares,
                0,
                block.timestamp,
                _receiver,
                _owner
            )
        );
    }

function safeRedeem(
        uint256 _shares,
        uint256 _minAmountOut, // Min_amount
        uint256 _deadline,
        address _receiver,
        address _owner
    ) external returns (uint256 assets) {
        return (
            _withdraw(
                convertToAssets(_shares), // _amount
                _shares, // _shares
                _minAmountOut,
                _deadline,
                _receiver, // _receiver
                _owner // _owner
            )
        );
    }
MEDIUM severity: [ASTRO-19] Missing fee limits check. **Severity:** Medium

**Path:** Crate.sol

**Description:** Unlike in the setFees function, the Crate contract constructor is setting the withdraw, performance and management fees, which are not being checked to be smaller than some amount (e.g. 10%); checks should exist ensuring that the fees are.....

Vulnerable code:
constructor(
        address _asset, // The asset we are using
        string memory _name, // The name of the token
        string memory _symbol, // The symbol of the token
        uint256 _performanceFee, // 100% = 10000
        uint256 _managementFee, // 100% = 10000
        uint256 _withdrawFee // 100% = 10000
    ) ERC20(_name, _symbol) {
        asset = IERC20(_asset);
        performanceFee = _performanceFee;
        managementFee = _managementFee;
        withdrawFee = _withdrawFee; 
        tokenDecimals = IERC20Metadata(_asset).decimals();
        checkpoint = Checkpoint(block.timestamp, 10 ** tokenDecimals);
        _pause(); // We start paused
    }
MEDIUM severity: [ASTRO-20] Incorrect withdraw preview calculations. **Severity:** Medium

**Path:** Crate.sol

**Description:** In the contract Crate, the withdraw preview function miscalculates how many shares are needed to withdraw a fixed amount of assets.

The _withdraw function calculates the output amount:

while previewFunction uses the following...

Vulnerable code:
_amount = (_amount * (MAX_BPS - withdrawFee)) / MAX_BPS;

(convertToShares(_assets) * (MAX_BPS + withdrawFee)) / MAX_BPS

convertToShares((_assets * MAX_BPS)/(MAX_BPS-withdrawfee)
LOW severity: [ASTRO-20] Redundant usage of SafeMath. **Severity:** Low

**Path:** Swap.sol 

**Description:** The contract uses Solidity `0.8.17`, which already checks arithmetic operations for over- and underflow. This is suboptimal and wastes gas on each function call containing these operations.

**Remediation:**  Remove the import of...

Vulnerable code:
import "@openzeppelin/contracts/utils/math/SafeMath.sol";
[...]
contract Swap is Ownable {
    using SafeMath for uint256;
    [...]
}
LOW severity: [ASTRO-1] Missing array parameters length check. **Severity:** Low

**Path:** Crate.sol, Allocator.sol

**Description:** In the following locations, there are at least 2 arrays as function parameters that require the same length but are missing these length checks:

- contracts/Crate.sol:dispatchAssets (L479-500)

-...

Vulnerable code:
contract Crate {

    [...]    

    function dispatchAssets(
        uint256[] calldata _amounts,
        uint256[] calldata _chainIds,
        uint256[] calldata _msgValues
    ) external payable onlyOwner {
        for (uint256 i = 0; i < _amounts.length; i++) {
            ChainData memory data = chainData[_chainIds[i]];
            // checks
            if (data.maxDeposit == 0) revert ChainError(); // Chain not active
            if (data.maxDeposit <= data.debt + _amounts[i])
                revert AmountTooHigh(data.maxDeposit); // No more funds can be sent to this chain

            chainData[_chainIds[i]].debt += _amounts[i];
            totalRemoteAssets += _amounts[i];
            asset.safeTransfer(data.bridge, _amounts[i]);
            if (block.chainid != _chainIds[i]) {
                IBridgeConnectorHome(data.bridge).bridgeFunds{
                    value: _msgValues[i]
                }(_amounts[i], _chainIds[i]);
            }
        }
    }
    
    [...]
    
    function estimateDispatchCost(
        uint256[] calldata _chainIds,
        uint256[] calldata _amounts
    ) external view returns (uint256[] memory) {
        uint256 length = _chainIds.length;
        uint256[] memory nativeCost = new uint256[](length);
        for (uint256 i; i < length; i++) {
            if (_chainIds[i] == block.chainid) continue;
            nativeCost[i] = IBridgeConnectorHome(chainData[_chainIds[i]].bridge)
                .estimateBridgeCost(_chainIds[i], _amounts[i]);
        }
        return (nativeCost);
    }
    
    [...]
  
  }
  
  Contract Allocator {
  
  [...]
  
  	function dispatchAssets(
		uint256[] calldata _amounts,  // @audit array lenghts
		address[] calldata _strategies
	) external onlyOwner {
		for (uint256 i; i < _amounts.length; i++) {
			isWhitelisted(_strategies[i]);

			uint256 debt = strategiesData[_strategies[i]].debt;

			if (strategiesData[_strategies[i]].maxDeposit <= debt + _amounts[i])
				revert MaxDepositReached(_strategies[i]);

			strategiesData[_strategies[i]].debt += _amounts[i];
			totalStrategysDebt += _amounts[i];

			IERC20(asset).safeTransfer(_strategies[i], _amounts[i]);

			emit StrategyUpdate(_strategies[i], debt += _amounts[i]);
		}

		emit ChainDebtUpdate(totalChainDebt());
	}
	
	[...]
	
	}

require(array1.length == array2.length, "Arrays length mismatch");
LOW severity: [ASTRO-14] Missing event on decreasing liquidity. **Severity:** Low

**Path:** Crate.sol

**Description:** The function decreaseLiquidity does not emit the event LiquidityChanged, in case there is such an event, in contrary to the function increaseLiquidity which does emit the event.

**Remediation:**  Add  LiquidityChanged event...

Vulnerable code:
function decreaseLiquidity(uint256 _liquidityRemoved) external onlyOwner {
        // Rebalance first the pool to avoid any negative slippage
        uint256 lpBal = liquidityPool.swap.getVirtualLpBalance();
        uint256 assetBalBefore = asset.balanceOf(address(this));
        uint256 liquidityBefore = liquidityPool.liquidity;

        // we remove liquidity
        liquidityPool.liquidity -= _liquidityRemoved;
        // We specify the amount of LP that corresponds to the amount of liquidity removed
        liquidityPool.swap.removeLiquidity(
            (lpBal * _liquidityRemoved) / liquidityBefore,
            block.timestamp
        );
        // We update the book
        liquidityPool.debt -= (asset.balanceOf(address(this)) - assetBalBefore);
    }
LOW severity: [ASTRO-11] Misleading error declaration. **Severity:** Informational

**Path:** bridgeConnectorRemoteSTG.sol

**Description:** The contract is declaring an error ZeroAddress() and using it in the function setAllocator()(L134-138), but that function doesn’t do a zero address check. Instead, it checks whether the allocator is set; thereby,.....

Vulnerable code:
contract BridgeConnectorRemoteSTG {

[...]

	error ZeroAddress();

[...]

	function setAllocator(address _allocator) external onlyOwner {
		// We can't change it after it's set
		if (allocator != address(0)) revert ZeroAddress();
		allocator = _allocator;
	}
}