Stader Labs - T1MOH's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L14 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L12 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/SocializingPool.sol#L17 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L17

Vulnerability details

Impact

Referenced contracts are unable to be paused as intended

Proof of Concept

Referenced contracts inherit PausableUpgradeable and implement the whenNotPaused modifier, but don't implement any method to actually pause or unpause the contract. PausableUpgradeable.sol only implements internal functions, which requires external or public functions to be implemented to wrap them. Since nothing like this has been implemented, the entire pausing system is rendered useless.

Tools Used

Manual review

Recommended Mitigation Steps

Create simple external pause and unpause functions that can be called by owner

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/PoolUtils.sol#L55-L65

Vulnerability details

Impact

Admin will never be able to update pool address

Proof of Concept

    function updatePoolAddress(uint8 _poolId, address _newPoolAddress)
        external
        override
        onlyExistingPoolId(_poolId)
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        UtilLib.checkNonZeroAddress(_newPoolAddress);
        verifyNewPool(_poolId, _newPoolAddress);
        poolAddressById[_poolId] = _newPoolAddress;
        emit PoolAddressUpdated(_poolId, _newPoolAddress);
    }

The function has two conflicting checks: modifier onlyExistingPoolId and function verifyNewPool. In first it requires isExistingPoolId() to return true, in second it requires to return false:

    modifier onlyExistingPoolId(uint8 _poolId) {
        if (!isExistingPoolId(_poolId)) {
            revert PoolIdNotPresent();
        }
        _;
    }
    function verifyNewPool(uint8 _poolId, address _poolAddress) internal view {
        if (
            INodeRegistry(IStaderPoolBase(_poolAddress).getNodeRegistry()).POOL_ID() != _poolId ||
            isExistingPoolId(_poolId)
        ) {
            revert ExistingOrMismatchingPoolId();
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Remove call to function verifyNewPool() in updatePoolAddress()

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The current implementation does not encourage bidders to submit bids at any time other than the very last second. It results in

1. Poor UX for bidders
2. And more importantly in underestimation of assets on Auction.

The purpose of the auction is to evaluate the asset and sell it at a fair price. But now it's a gas war to put your transaction as the last one in the block.

Proof of Concept

As you can see it does not extend lotItem.endBlock if bid was submitted too close to the end

    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);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

For example extend auction by 30 minutes if the bid was submitted <30 minutes before the end. Make sure to refactor bidIncrement logic, because in current implementation it is constant 0.005ETH, therefore creates issue of infinite extending of auction at low cost (4 days for 1 ETH).

Assessed type

MEV

Allow bidders to withdraw not highest bid when auction is not ended

File: contracts/Auction.sol

Remove the check that the auction has ended. It blocks users' funds and doesn't make any sense

    function withdrawUnselectedBid(uint256 lotId) external override nonReentrant {
        LotItem storage lotItem = lots[lotId];
        if (block.number <= lotItem.endBlock) revert AuctionNotEnded(); //@audit
        if (msg.sender == lotItem.highestBidder) revert BidWasSuccessful();

        ...
    }

Add additional check in createLot() that msg.sender is SDCollateral.sol

Add this check to prevent users from interacting with system contract and following losing of SD tokens. Because ETH and SD tokens will be transferred to StaderTreasury and StaderStakePoolsManager.sol

    function createLot(uint256 _sdAmount) external override whenNotPaused {
        //SOLUTION
        require(msg.sender == staderConfig.getSDCollateral());

        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++;
    }