code423n4 commented 2 years ago

Handle

leastwood

Vulnerability details

Impact

The ConvexYieldWrapper.sol contract makes use of a user's total collateral held by all their vaults, however, there is no check to ensure the vault is sufficiently collateralised. Hence, it is possible for a user to claim protocol generated yield on an undercollateralised vault.

As per the _getDepositedBalance function, there are currently only checks to ensure that account_ is the owner before tallying the total collateral held by all owned vaults.

Proof of Concept

https://github.com/yieldprotocol/vault-v2/blob/master/contracts/Cauldron.sol#L477-L495

function _level(
    DataTypes.Vault memory vault_,
    DataTypes.Balances memory balances_,
    DataTypes.Series memory series_
)
    internal
    returns (int256)
{
    DataTypes.SpotOracle memory spotOracle_ = spotOracles[series_.baseId][vault_.ilkId];
    uint256 ratio = uint256(spotOracle_.ratio) * 1e12;   // Normalized to 18 decimals
    (uint256 inkValue,) = spotOracle_.oracle.get(vault_.ilkId, series_.baseId, balances_.ink);    // ink * spot

    if (uint32(block.timestamp) >= series_.maturity) {
        uint256 accrual_ = _accrual(vault_.seriesId, series_);
        return inkValue.i256() - uint256(balances_.art).wmul(accrual_).wmul(ratio).i256();
    }

    return inkValue.i256() - uint256(balances_.art).wmul(ratio).i256();
}

https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexYieldWrapper.sol#L100-L120

function _getDepositedBalance(address account_) internal view override returns (uint256) {
    if (account_ == address(0) || account_ == collateralVault) {
        return 0;
    }

    bytes12[] memory userVault = vaults[account_];

    //add up all balances of all vaults registered in the wrapper and owned by the account
    uint256 collateral;
    DataTypes.Balances memory balance;
    uint256 userVaultLength = userVault.length;
    for (uint256 i = 0; i < userVaultLength; i++) {
        if (cauldron.vaults(userVault[i]).owner == account_) {
            balance = cauldron.balances(userVault[i]);
            collateral = collateral + balance.ink;
        }
    }

    //add to balance of this token
    return _balanceOf[account_] + collateral;
}

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider adding a cauldron.level(userVault[i]) >= 0 check to _getDepositedBalance to ensure only collateralised vaults are included in this calculation. However, it is important to note that if a vault was previously checkpointed as being collateralised, but becomes undercollateralised, any vault liquidations are reflected in the _getDepositedBalance calculation.

alcueca commented 2 years ago

If a vault becomes undercollateralised it will be captured by the liquidations engine pretty soon, until then we are ok with giving rewards.

Once the vault is captured by the liquidations engine it is owned by the liquidations engine and its collateral is not counted towards any rewards (unless someone checkpoints rewards for the liquidations engine, which is fine).

Once the vault is liquidated it is given back to the account. Before any rewards operation can be done, the rewards will be checkpointed again.

GalloDaSballo commented 2 years ago

I believe there's merit to this finding, given higher severity findings related to transferring vaults.

However, this warden has submitted those higher severity findings, and for this one, a lack of clear POC makes it hard to verify whether this is an actual vulnerability.

Ultimately the rewards for the vault should be accrued even if the vault is undercollateralized, so given this rationale, I'll side with the sponsor and mark this invalid

code-423n4 / 2022-01-yield-findings