LSD Network - Stakehouse contest - Franfran's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L278 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L280 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L687-L689 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L267-L273

Vulnerability details

Impact

In the updateNodeRunnerWhitelistStatus() function, nobody can get whitelisted because of a wrong checking. If the DAO enableWhitelisting functionality is enabled, then no new node runner will be able to register a new BLS key, this would partly freeze the system.

Proof of Concept

function setUp() public {
    vm.startPrank(accountFive); // this will mean it gets dETH initial supply
    factory = createMockLSDNFactory();
    vm.stopPrank();

    // Deploy 1 network and get default dependencies
    manager = deployNewLiquidStakingNetwork(
        factory,
        admin,
        true,
        "LSDN"
    );

    savETHVault = getSavETHVaultFromManager(manager);
    stakingFundsVault = getStakingFundsVaultFromManager(manager);

    // make 'admin' the 'DAO'
    vm.prank(address(factory));
    manager.updateDAOAddress(admin);
}

function testCannotWhitelist() public {
    // Set up users and ETH
    address nodeRunner = accountOne; vm.deal(nodeRunner, 4 ether);
    // Node runner is not yet whitelisted
    bool wl = manager.isNodeRunnerWhitelisted(nodeRunner);

    // Prank as DAO
    vm.startPrank(admin);

    // Check by banning
    vm.expectRevert(bytes("Unnecessary update to same status"));
    manager.updateNodeRunnerWhitelistStatus(nodeRunner, false);

    // Check with true ?
    vm.expectRevert(bytes("Unnecessary update to same status"));
    manager.updateNodeRunnerWhitelistStatus(nodeRunner, true);

    // Both revert
}

Tools Used

Manual inspection

Recommended Mitigation Steps

Use the isWhitelisted value to check against the isNodeRunnerWhitelisted instead.

function updateNodeRunnerWhitelistStatus(address _nodeRunner, bool isWhitelisted) external onlyDAO {
    require(_nodeRunner != address(0), "Zero address");
    // - require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");
    + require(isWhitelisted != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");

    isNodeRunnerWhitelisted[_nodeRunner] = isWhitelisted;
    emit NodeRunnerWhitelistingStatusChanged(_nodeRunner, isWhitelisted);
}

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L356 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L369

Vulnerability details

Impact

As the rotateNodeRunnerOfSmartWallet function can be called by anyone who is a node runner in the LSD network, this function is vulnerable to a frontrun attack in the case of this node runner being malicious.

Proof of Concept

If that is the current node runner is malicious, the DAO would purposely call this same rotateNodeRunnerOfSmartWallet with the _wasPreviousNodeRunnerMalicious flag turned on. An actual node runner that has been malicious could monitor the mempool and frontrun the DAO transaction that wanted to slash it and submit the transaction before the DAO to avoid getting banned and rotate their EOA representation of the node.

if (msg.sender == dao && _wasPreviousNodeRunnerMalicious) {
    bannedNodeRunners[_current] = true;
    emit NodeRunnerBanned(_current);
}

When the DAO transaction would go through, it would revert when it's checking if the current (old) node representative is still a wallet, but it's not because the mapping value has been deleted before.

address wallet = smartWalletOfNodeRunner[_current];
require(wallet != address(0), "Wallet does not exist");

Tools Used

Manual inspection

Recommended Mitigation Steps

Restrict this function to DAO only with the onlyDAO modifier.

// - function rotateNodeRunnerOfSmartWallet(address _current, address _new, bool _wasPreviousNodeRunnerMalicious) external {
+ function rotateNodeRunnerOfSmartWallet(address _current, address _new, bool _wasPreviousNodeRunnerMalicious) onlyDAO external {
    require(_new != address(0) && _current != _new, "New is zero or current");

    address wallet = smartWalletOfNodeRunner[_current];
    require(wallet != address(0), "Wallet does not exist");
    require(_current == msg.sender || dao == msg.sender, "Not current owner or DAO");

    address newRunnerCurrentWallet = smartWalletOfNodeRunner[_new];
    require(newRunnerCurrentWallet == address(0), "New runner has a wallet");

    smartWalletOfNodeRunner[_new] = wallet;
    nodeRunnerOfSmartWallet[wallet] = _new;

    delete smartWalletOfNodeRunner[_current];

    // - if (msg.sender == dao && _wasPreviousNodeRunnerMalicious) {
    if (_wasPreviousNodeRunnerMalicious) {
        bannedNodeRunners[_current] = true;
        emit NodeRunnerBanned(_current);
    }

    emit NodeRunnerOfSmartWalletRotated(wallet, _current, _new);
}

1 _addPriorityStakers() doesn't enforce duplicate array elements

Issue

_addPriorityStakers() doesn't revert if array elements are equal when next to each other. Even though this can be written to publicly (when deploying a syndicate with deploySyndicate in SyndicateFactory.sol), they are no side effect as the same staker will only be written to true twice in the isPriorityStaker mapping, so submitted as low severity.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L583

POC

contract Syndicate is Test {
    function _addPriorityStakers(address[] memory _priorityStakers) internal {
        if (_priorityStakers.length == 0) revert EmptyArray();
        for (uint256 i; i < _priorityStakers.length; ++i) {
            address staker = _priorityStakers[i];

            if (i > 0 && staker < _priorityStakers[i-1]) revert DuplicateArrayElements();

            // isPriorityStaker[staker] = true;

            // emit PriorityStakerRegistered(staker);
        }
    }

    address r1 = address(123456789);
    address r2 = address(12345678910);

    function testDuplicate() public {
        address[] memory s1 = new address[](0);
        vm.expectRevert(EmptyArray.selector);
        _addPriorityStakers(s1);

        address[] memory s2 = new address[](2);
        s2[0] = r1;
        s2[1] = r2;
        // normal behaviour, shouldn't revert
        _addPriorityStakers(s2);

        address[] memory s3 = new address[](3);
        s3[0] = r1;
        s3[1] = r2;
        s3[2] = r1;
        // duplicate case on address not next to each other, should revert
        vm.expectRevert(DuplicateArrayElements.selector);
        _addPriorityStakers(s3);

        address[] memory s4 = new address[](3);
        s4[0] = r1;
        s4[1] = r2;
        s4[2] = r2;
        // should revert, but does not
        _addPriorityStakers(s4);
    }
}

Remedy

Should use if (i > 0 && staker <= _priorityStakers[i-1]) revert DuplicateArrayElements(); instead.