Description

In the main loop inside the unbond function, the PolygonStrategy contract checks to ensure that the quantity it will claim as rewards from the external staking contract is above the minimum claim amount:

function unbond(uint256 _toUnbond) external onlyFundFlowController {
    // [...]
    while (toUnbondRemaining != 0) {
        // [...]
        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;
            } // [...]

The above code assumes that if the comparison rewards >= vault.minRewardClaimAmount() passes, then calling vault.withdrawRewards will not revert.

However, the rewards variable is set to deposits - principalDeposits. The getTotalDeposits function in the PolygonVault contract has the following implementation:

function getTotalDeposits() public view returns (uint256) {
    return
        getPrincipalDeposits() +
        getRewards() +
        getQueuedWithdrawals() +
        token.balanceOf(address(this));
}
// [...]

There are four components to this calculation:

1. The second call to getPrincipalDeposits() likely returns the same quantity as the first one, so that subtraction cancels out. Unless there is an upgrade to the upstream vault, there is no reentrancy site for it to have changed.

2. The call to getQueuedWithdrawals() will return zero, because the vault will not be unbonding at this time, unless upgrades made to the upstream vault break the invariant implemented, numVaultsUnbonding.

3. The call to getRewards() will return the rewards quantity that is the same one used by the upstream's minimum check.

4. The call to token.balanceOf, however, is a free parameter and can be arbitrarily inflated by the attacker.

In the upstream vault implementation↗, the implementation of this minimum is this:

function _withdrawRewards(bool pol) internal {
    uint256 rewards = _withdrawAndTransferReward(msg.sender, pol);
    require(rewards >= minAmount, "Too small rewards amount");
}

// [...]

function _withdrawAndTransferReward(address user, bool pol) private returns (uint256) {
    uint256 liquidRewards = _withdrawReward(user);
    // [...]
    return liquidRewards;
}

// [...]

function _withdrawReward(address user) private returns (uint256) {
    // [...]
    uint256 liquidRewards = _calculateReward(user, _rewardPerShare);
    // [...]
    return liquidRewards;
}

So the call to token.balanceOf should be excluded from this check, because the token balance is not considered in the upstream's minimum check.

Impact

An attacker can cause some calls to unbond to revert.

More specifically, if a call to unbond considers skipping the withdrawal of rewards from a vault whose rewards are too little to claim on the upstream, then an attacker who is positioned to front-run unbond can donate tokens to the vault in order to meet that minimum. This will cause the reward claim to be attempted and therefore revert the original call to unbond.

This revert is inconvenient because the same unbond cannot be attempted again, since it will revert again. The only fixes to this state would be 1) waiting for enough rewards to accrue naturally so that the withdrawal of rewards succeeds or 2) manual intervention by governance.

Recommendations

Correct this calculation by calling getRewards, which calls getLiquidRewards, directly to check this threshold.

In the upstream's implementation, the getLiquidRewards function directly returns the result of _calculateReward, which is the same quantity used by the minimum check, as seen above.

function getLiquidRewards(address user) public view returns (uint256) {
    return _calculateReward(user, getRewardPerShare());
}

Additionally, in line with Finding ref↗, we recommend separately handling the three steps of a vault unbond — withdrawing the standing token balance, claiming the rewards, and unstaking the claim.

Remediation

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