code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

medium

Title: The implementation of the withdrawal logic inside SDCollateral is missing the timelock mechanism

Impact

The current implementation of the withdraw() function inside SDCollateral lacks a timelock mechanism. As a result, the node operator can withdraw their SD tokens without any verification, potentially evading slashing.


    /// @notice for operator to request withdraw of sd
    /// @dev it does not transfer sd tokens immediately
    /// operator should come back after withdrawal-delay time to claim
    /// this requested sd is subject to slashes
    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);
    }

The code snippet lacks the necessary logic to verify whether the timelock period for the withdrawal has expired,
despite the comment suggesting its existence.

Proof of Concept

The Withdraw function in SDCollateral solely checks if the opSDBalance (Node operator's collateral balance)
is less than the sum of the operator's withdrawal threshold and the requested withdrawal amount.
Notably, it lacks a check for a withdrawal timelock:

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

        if (opSDBalance < getOperatorWithdrawThreshold(operator) + _requestedSD) { // only revert
            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);
    }

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

This check triggers a revert only if a Node operator attempts to withdraw more tokens than their balance allows.
However, the absence of a withdrawal timelock check enables any malicious Node operator to withdraw their collateral before experiencing any potential slashing consequences.

Recommended Mitigation Steps

Add timelock.

Assessed type

Token-Transfer

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #180