Description

In the unbond function, this logic handles deciding what to do with the vault that is next in line for unbonding — specifically, whether to skip it, withdraw rewards from it, or unbond it:

IPolygonVault vault = vaults[i];
uint256 deposits = vault.getTotalDeposits();

if (deposits != 0) {
    uint256 principalDeposits = vault.getPrincipalDeposits();
    uint256 rewards = deposits - principalDeposits;

    if (rewards >= toUnbondRemaining && rewards >= vault.minRewardClaimAmount()) {
        vault.withdrawRewards();
        toUnbondRemaining = 0;
        break;
    } else if (principalDeposits != 0) {
        if (toUnbondRemaining > rewards) {
            toUnbondRemaining -= rewards;
        }

        uint256 vaultToUnbond = principalDeposits >= toUnbondRemaining
            ? toUnbondRemaining
            : principalDeposits;

        vault.unbond(vaultToUnbond);

        toUnbondRemaining -= vaultToUnbond;
        ++numVaultsUnbonded;
    }
}

The if (toUnbondRemaining > rewards) { statement runs in two different cases, because of the previous conditional. In the case where the previous conditional fails due to rewards >= toUnbondRemaining being false, then the toUnbondRemaining -= rewards; line always runs, which is appropriate. But in the case where the previous conditional fails due to the minRewardClaimAmount check, this logic does not deduct rewards from the quantity to be unbonded.

This is a misaccounting, because according to the upstream vault implementation, when the vault is unbonded, the rewards are claimed. So the unbond function will unbond more funds than necessary.

Impact

The unbond function will unbond more funds than necessary when satisfying an unbond request. This will slightly decrease the rewards rate and also cause the vaults that are next in line to be unduly unbonded earlier than needed.

Recommendations

We recommend separating this logic into three stages when calculating whether to withdraw or unbond a vault.

First, the vault's standing balance should be checked. These are tokens that are not staked to the underlying vault and therefore are freely transferrable back to the PolygonStrategy to be queued. We recommend unconditionally withdrawing and requeueing these funds, since these funds will typically only be the result of attackers directly donating vault shares into the vault (a donation of any amount will claim rewards into the vault), and they should be accounted and requeued immediately. Even if the toUnbondRemaining is less, it should still claim the full amount, and then exit.

Secondly, the vault's withdrawable rewards should be checked. These are the unclaimed rewards that can only be directly withdrawn if the amount is greater than the minimum. We did not analyze the benefits or drawbacks of immediately unconditionally withdrawing the rewards here, but a decision should be made whether to withdraw them.

Finally, the vault's principal deposits should be checked. If the previous step did not withdraw the rewards for any reason (e.g., the quantity is below the minimum, or it is not economical to withdraw it), then the amount of those rewards should be added to the amount we will get if we unbond any quantity of principal, since the logic in the upstream contract implements it like that.

In between each of these three steps, the toUnbondRemaining should be reduced either to zero or by the amount that is released.

Remediation

This issue has been acknowledged by Stake.link, and a fix was implemented in commit ce454ac1↗.