Stader Labs - DadeKuma's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L62-L78

Vulnerability details

Impact

MEV bots may bid all the ongoing auctions before Auction is paused by frontrunning it with the addBid function.

This can lead to bots winning all the auctions for a fraction of their price, especially auctions that are almost ended, as no one is able to bid until the contract is unpaused.

Proof of Concept

No one will be able to bid when the contract is paused, but bots can simply front-run the pause by looking at the mempool:

	function addBid(uint256 lotId) external payable override whenNotPaused {
	    // reject payments of 0 ETH
	    if (msg.value == 0) revert InSufficientETH();

	    LotItem storage lotItem = lots[lotId];
	    if (block.number > lotItem.endBlock) revert AuctionEnded();

	    uint256 totalUserBid = lotItem.bids[msg.sender] + msg.value;

	    if (totalUserBid < lotItem.highestBidAmount + bidIncrement) revert InSufficientBid();

	    lotItem.highestBidder = msg.sender;
	    lotItem.highestBidAmount = totalUserBid;
	    lotItem.bids[msg.sender] = totalUserBid;

	    emit BidPlaced(lotId, msg.sender, totalUserBid);
	}

A word about severity; the contract extends PausableUpgradeable:

	contract Auction is IAuction, Initializable, AccessControlUpgradeable, PausableUpgradeable, ReentrancyGuardUpgradeable

Also, it has several functions that have the whenNotPaused modifier:

	function createLot(uint256 _sdAmount) external override whenNotPaused

	function addBid(uint256 lotId) external payable override whenNotPaused

However, it doesn't have any pause/unpause functions that are external/public, as they have internal visibility by default in PausableUpgradeable; as such, the contract can't be paused with the current implementation.

However, this may change in the future as the contract is upgradeable, so I still consider this issue valid.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the whenNotPaused modifier from addBid, so ongoing auctions may be ended even when the contract is paused.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L646-L649

Vulnerability details

Impact

latestRoundData may return stale/wrong data but there aren't any checks to ensure that this happen.

This impacts the balance, supply, and exchange rate that will be used to make withdrawals/deposits.

Proof of Concept

There are some checks missing from the Chainlink response (round completeness, staleness, price):

	function getPORFeedData()
	    internal
	    view
	    returns (
	        uint256,
	        uint256,
	        uint256
	    )
	{
	    (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
	        .latestRoundData();
	    (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
	        .latestRoundData();
	    return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
	}

Issues with Chainlink, such as oracle consensus problems or chain congestion, may result in this contract relying on outdated data.

As these values are used to calculate the exchange rate, it may result in an incorrect price:

function updateERFromPORFeed() external override checkERInspectionMode whenNotPaused {
    if (!isPORFeedBasedERData) {
        revert InvalidERDataSource();
    }
    (uint256 newTotalETHBalance, uint256 newTotalETHXSupply, uint256 reportingBlockNumber) = getPORFeedData();
    updateWithInLimitER(newTotalETHBalance, newTotalETHXSupply, reportingBlockNumber);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding the following checks:

    (uint80 roundETHBalance, int256 totalETHBalanceInInt, , uint timeStampETHBalance, uint80 answerRoundETHBalance) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
		        .latestRoundData();
    (uint80 roundETHSupply, int256 totalETHXSupplyInInt, , uint timeStampETHSupply, uint80 answerRoundETHSupply) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
        .latestRoundData();

    require(totalETHBalanceInInt > 0, "Negative Oracle Price");
    require(totalETHXSupplyInInt > 0, "Negative Oracle Price");

    require(timeStampETHBalance >= block.timestamp, "Stale pricefeed");
    require(timeStampETHSupply >= block.timestamp, "Stale pricefeed");

    require(roundETHBalance >= answerRoundETHBalance, "Round not complete");
    require(roundETHSupply >= answerRoundETHSupply, "Round not complete");

Assessed type

Invalid Validation

Findings

• [L-01] Missing access control in createLot
• [L-02] Additional penalty can't be decreased
• [L-03] Non-onboarded operators can't be removed from the whitelist
• [L-04] Reward events can be emitted by anyone
• [L-05] Missing access control in operator deposit
• [L-06] Users may trigger the vault deposit with zero assets

[L-01] Missing access control in createLot

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L48-L60

Anyone can create an auction, but they will get nothing in return as their funds will be lost. This function should be callable only by SDCollateral:

	function createLot(uint256 _sdAmount) external override whenNotPaused {
	    lots[nextLot].startBlock = block.number;
	    lots[nextLot].endBlock = block.number + duration;
	    lots[nextLot].sdAmount = _sdAmount;

	    LotItem storage lotItem = lots[nextLot];

	    if (!IERC20(staderConfig.getStaderToken()).transferFrom(msg.sender, address(this), _sdAmount)) {
	        revert SDTransferFailed();
	    }
	    emit LotCreated(nextLot, lotItem.sdAmount, lotItem.startBlock, lotItem.endBlock, bidIncrement);
	    nextLot++;
	}

[L-02] Additional penalty can't be decreased

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Penalty.sol#L53-L59

The penalty amount can only increase, but not decrease. If this function is called on an intended _pubkey, it won't be possible to decrease it again:

	function setAdditionalPenaltyAmount(bytes calldata _pubkey, uint256 _amount) external override {
	    UtilLib.onlyManagerRole(msg.sender, staderConfig);
	    bytes32 pubkeyRoot = UtilLib.getPubkeyRoot(_pubkey);
	    additionalPenaltyAmount[pubkeyRoot] += _amount;

	    emit UpdatedAdditionalPenaltyAmount(_pubkey, _amount);
	}

[L-03] Non-onboarded operators can't be removed from the whitelist

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PermissionedNodeRegistry.sol#L88-L97

If an unintended address is added to the operator whitelist by mistake, it's not possible to remove it before they complete the onboarding process:

	function whitelistPermissionedNOs(address[] calldata _permissionedNOs) external override {
	    UtilLib.onlyManagerRole(msg.sender, staderConfig);
	    uint256 permissionedNosLength = _permissionedNOs.length;
	    for (uint256 i = 0; i < permissionedNosLength; i++) {
	        address operator = _permissionedNOs[i];
	        UtilLib.checkNonZeroAddress(operator);
	        permissionList[operator] = true;
	        emit OperatorWhitelisted(operator);
	    }
	}

[L-04] Reward events can be emitted by anyone

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L78-L80

There are multiple payable functions that are not protected by access control. Anyone can send 1 wei to trigger these functions, which may be used to trick other components that listen to these events.

For example, this event should be emitted only when the withdraw vault completes a payment, but it's callable by anyone:

	// payable function for receiving user share from validator withdraw vault
	function receiveWithdrawVaultUserShare() external payable override {
	    emit WithdrawVaultUserShareReceived(msg.value);
	}

[L-05] Missing access control in operator deposit

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SDCollateral.sol#L43-L52

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/SDCollateral.sol#L58-L73

Anyone can send funds as a deposit, but there isn't an access control to check if they are an operator:

	function depositSDAsCollateral(uint256 _sdAmount) external override { 
	    address operator = msg.sender;
	    operatorSDBalance[operator] += _sdAmount;

	    if (!IERC20(staderConfig.getStaderToken()).transferFrom(operator, address(this), _sdAmount)) {
	        revert SDTransferFailed();
	    }

	    emit SDDeposited(operator, _sdAmount);
	}

When these users try to withdraw their funds and they aren't an operator, their transaction will fail, and their funds will be locked:

	function withdraw(uint256 _requestedSD) external override {
	    address operator = msg.sender;
	    uint256 opSDBalance = operatorSDBalance[operator];

->	    if (opSDBalance < getOperatorWithdrawThreshold(operator) + _requestedSD) {
	        revert InsufficientSDToWithdraw(opSDBalance);
	    }
	    operatorSDBalance[operator] -= _requestedSD;

	    // cannot use safeERC20 as this contract is an upgradeable contract
	    if (!IERC20(staderConfig.getStaderToken()).transfer(payable(operator), _requestedSD)) {
	        revert SDTransferFailed();
	    }

	    emit SDWithdrawn(operator, _requestedSD);
	}

[L-06] Users may trigger the vault deposit with zero assets

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L169-L177

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/StaderStakePoolsManager.sol#L154-L156

No one can do a deposit unless their assets are greater than the minDeposit:

	function deposit(address _receiver) public payable override whenNotPaused returns (uint256) {
	    uint256 assets = msg.value;
	    if (assets > maxDeposit() || assets < minDeposit()) {
	        revert InvalidDepositAmount();
	    }
	    uint256 shares = previewDeposit(assets);
	    _deposit(msg.sender, _receiver, assets, shares);
	    return shares;
	}

However, when the vault is unhealthy, the minDeposit is zero:

	function minDeposit() public view override returns (uint256) {
	    return isVaultHealthy() ? staderConfig.getMinDepositAmount() : 0;
	}

As such, the previous condition will not revert and the user will be able to call the deposit function without failing.

Consider adding a new check:

	if (assets == 0 || assets > maxDeposit() || assets < minDeposit()) {
	    revert InvalidDepositAmount();
	}