HIGH: SLOT owner claims are incorrect when there is only 1 slot owner.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L511

Vulnerability details

Description

In Syndicate.sol, _updateCollateralizedSlotOwnersLiabilitySnapshot function calculates accruedEarningPerCollateralizedSlotOwnerOfKnot for each slot owner. Later this variable is used to satisfy claims in claimAsCollateralizedSLOTOwner().

We can take a look at the following code:

if (currentSlashedAmount < 4 ether) {
    // This copes with increasing numbers of collateralized slot owners and also copes with SLOT that has been slashed but not topped up
    uint256 numberOfCollateralisedSlotOwnersForKnot = getSlotRegistry().numberOfCollateralisedSlotOwnersForKnot(_blsPubKey);
    if (numberOfCollateralisedSlotOwnersForKnot == 1) {
        // For only 1 collateralized SLOT owner, they get the full amount of unprocessed ETH for the knot
        address collateralizedOwnerAtIndex = getSlotRegistry().getCollateralisedOwnerAtIndex(_blsPubKey, 0);
        accruedEarningPerCollateralizedSlotOwnerOfKnot[_blsPubKey][collateralizedOwnerAtIndex] += unprocessedETHForCurrentKnot;
    } else {
        for (uint256 i; i < numberOfCollateralisedSlotOwnersForKnot; ++i) {
            address collateralizedOwnerAtIndex = getSlotRegistry().getCollateralisedOwnerAtIndex(_blsPubKey, i);
            uint256 balance = getSlotRegistry().totalUserCollateralisedSLOTBalanceForKnot(
                stakeHouse,
                collateralizedOwnerAtIndex,
                _blsPubKey
            );
            accruedEarningPerCollateralizedSlotOwnerOfKnot[_blsPubKey][collateralizedOwnerAtIndex] +=
                balance * unprocessedETHForCurrentKnot / (4 ether - currentSlashedAmount);
        }
    }
    // record so unprocessed goes to zero
    totalETHProcessedPerCollateralizedKnot[_blsPubKey] = accumulatedETHPerCollateralizedSlotPerKnot;
}

In the block inside if (numberOfCollateralisedSlotOwnersForKnot == 1), accruedEarningPerCollateralizedSlotOwnerOfKnot of the only owner is incremented by unprocessedETHForCurrentKnot.

However, if there are several slot owners, each one is credited a relative portion of unprocessedETHForCurrentKnot, divided by (4 ether - currentSlashedAmount). This is an issue since the Syndicate does not take into account the slashedAmount if there is only one owner.

We can confirm this is inconsistent with the preview function:

for (uint256 i; i < numberOfCollateralisedSlotOwnersForKnot; ++i) {
    address collateralizedOwnerAtIndex = getSlotRegistry().getCollateralisedOwnerAtIndex(_blsPubKey, i);
    if (collateralizedOwnerAtIndex == _staker) {
        uint256 balance = getSlotRegistry().totalUserCollateralisedSLOTBalanceForKnot(
            stakeHouse,
            collateralizedOwnerAtIndex,
            _blsPubKey
        );
        if (currentSlashedAmount < 4 ether) {
            currentAccrued +=
                balance * unprocessedForKnot / (4 ether - currentSlashedAmount);
        }
        break;
    }
}

Since we don't divide by (4 ether - currentSlashedAmount), it is the same behavior as currentSlashedAmount = 3 ether, which means SLOT owner may receive either much more or much less than they should when claiming.

Impact

SLOT owner claims are incorrect when there is only 1 slot owner.

Proof of Concept

Tools Used

Manual audit

Recommended Mitigation Steps

Change the calculation to be consistent in both scenarios.

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

vince0656 marked the issue as sponsor disputed

[vince0656]

I disputed it is because for a single collateralized SLOT owner you don't need to issue pro-rata ETH until the SLOT is topped up (see Stakehouse protocol for more info). For example:

• ETH accrued in execution layer rewards: 0.5
• ETH that can be claimed by free floating: 0.25
• ETH that can be claimed by collateralized: 0.25

if there is only 1 collateralized owner, then it gets 0.25 even if there is a slashing. it's only after a top up that the ETH can be split because then the contract knows who to allocate the ETH to. For a savvy bot, if you top up before a claim is made to the syndicate then they will get a % of the 0.25 because then the collateralized owners goes to 2 rather than one where their balance will then determine what proportion.

[c4-judge]

dmvt marked the issue as nullified

[trust1995]

The report explains that there is inconsistent calculation between claimAsCollateralizedSLOTOwner and previewUnclaimedETHAsCollateralizedSlotOwner. The sponsor's explanation, also not 100% clear to me, reasons that claimAsCollateralizedSLOTOwner() is calculating as intended. So if that is so, the preview function is miscalculating. Suppose currentSlashAmount = 0.
Then preview divides by 4:

if (currentSlashedAmount < 4 ether) {
            currentAccrued +=
                balance * unprocessedForKnot / (4 ether - currentSlashedAmount);
        }

But claim uses unproceessedForKnot without division:

if (numberOfCollateralisedSlotOwnersForKnot == 1) {
        // For only 1 collateralized SLOT owner, they get the full amount of unprocessed ETH for the knot
        address collateralizedOwnerAtIndex = getSlotRegistry().getCollateralisedOwnerAtIndex(_blsPubKey, 0);
        accruedEarningPerCollateralizedSlotOwnerOfKnot[_blsPubKey][collateralizedOwnerAtIndex] += unprocessedETHForCurrentKnot;
    }

So preview function shows a quarter of the actual accrued rewards, when there is 1 slot owner. In this case, severity should be dropped to M because function will lie to user, but eventually does not actually give them less funds than supposed to.

[vince0656]

@trust1995 thank you for pointing out the preview function. it should behave more like the state changing function

[c4-judge]

dmvt marked the issue as not nullified

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b