Description

The getPrincipalDeposits function in the PolygonVault contract uses the exchange rate to get the current rebasing amount that the principal deposit shares are worth:

function getPrincipalDeposits() public view returns (uint256) {
    return
        (validatorPool.balanceOf(address(this)) * validatorPool.exchangeRate()) /
        _getRatePrecision();
}

// [...]

function _getRatePrecision() private view returns (uint256) {
    uint256 validatorId = validatorPool.validatorId();

    // if foundation validator, use old precision
    if (validatorId < 8) {
        return EXCHANGE_RATE_PRECISION;
    }

    return EXCHANGE_RATE_HIGH_PRECISION;
}

The reimplementation of the _getRatePrecision function is copied from the upstream ValidatorShare contract's implementation. This means that this PolygonVault implementation assumes that the rate position will never change, which is not a feature that the upstream contract reasonably guarantees.

Impact

There is no current impact at the time of writing. However, if the implementation of Valuableshare changes in the future, the assumptions about the ratio position may be broken.

Recommendations

We recommend calling getTotalStake on the ValidatorShare contract in order to rely on it to calculate the exchange rate with the correct precision.

The current implemented version of getTotalStake matches the above calculation:

function getTotalStake(address user) public view returns (uint256, uint256) {
    uint256 shares = balanceOf(user);
    uint256 rate = exchangeRate();
    if (shares == 0) {
        return (0, rate);
    }

    return (rate.mul(shares).div(_getRatePrecision()), rate);
}

So, as of now, there would be no difference between the two implementations. But in the future, if the ValidatorShare implementation changes, then calling the public view function to get the total stake is safer than making internal assumptions about the rate precision.

Remediation

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