Gro Protocol contest - cmichel's results

Handle

cmichel

Vulnerability details

Vulnerability Details

The safetyCheck function performs an unsafe subtraction on two uint256 before casting them to int256. The subtraction can underflow and the cast to int256 can either fail and revert the transaction (if greater than type(int256).max), or, fit into an int256 and corrupt the safetyCheck making it always return false.

// _ratio - lastRatio[i] are uint256s and underflows
_ratio = abs(int256(_ratio - lastRatio[i]));

If the lastRatio[i] is even just 1 "wei" less than _ratio, the result will be type(uint256).max and the cast to int256 will fail due to the size limit of signed integers. All functions implementing the safetyCheck will revert and the protocol can become stuck and unusable.

Recommended Mitigation Steps

As only the absolute value is relevant the following code should work without having to cast to int256:

uint256 ratioDiff = _ratio > lastRatio[i] ? _ratio - lastRatio[i] : lastRatio[i] - _ratio;

Handle

cmichel

Vulnerability details

Vulnerability Details

The safetyCheck function has several issues that impact how precise the checks are:

1. only checks if the a/b and a/c ratios are within BASIS_POINTS. By transitivity b/c is only within 2 * BASIS_POINTS if a/b and a/c are in range. For a more precise check whether both USDC and USDT are within range, b/c must be checked as well.

2. If a/b is within range, this does not imply that b/a is within range.

"inverted ratios, a/b bs b/a, while producing different results should both reflect the same change in any one of the two underlying assets, but in opposite directions"

Example: lastRatio = 1.0 ratio: a = 1.0, b = 0.8 => a/b = 1.25, b/a = 0.8 If a/b was used with a 20% range, it'd be out of range, but b/a is in range.

3. The natspec for the function states that it checks Curve and an external oracle, but no external oracle calls are checked, both _ratio and lastRatio are only from Curve. Only _updateRatios checks the oracle.

Recommended Mitigation Steps

In addition, check if b/c is within BASIS_POINTS.

Handle

cmichel

Vulnerability details

Vulnerability Details

The Chainlink API (latestAnswer) used in the Buoy3Pool oracle wrappers is deprecated:

This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs

Impact

It seems like the old API can return stale data. Checks similar to that of the new API using latestTimestamp and latestRoundare are needed. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

Recommended Mitigation Steps

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = chainlink.latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

Handle

cmichel

Vulnerability details

Vulnerability Details

The two BaseVaultAdaptor.calculateShare functions computes share = amount.mul(uint256(10)**decimals).div(sharePrice)

uint256 sharePrice = _getVaultSharePrice();
// amount is in "token" decimals, share should be in "vault" decimals
share = amount.mul(uint256(10)**decimals).div(sharePrice);

This assumes that the sharePrice is always in token decimals and that token decimals is the same as vault decimals.

This both happens to be the case for Yearn vaults, but will not necessarily be the case for other protocols. As this functionality is in the BaseVaultAdaptor and not in the specific VaultAdaptorYearnV2_032, consider generalizing the conversion.

Impact

Integrating a token where the token or price is reported in a different precision will lead to potential losses as more shares are computed.

Recommended Mitigation Steps

The conversion seems highly protocol specific, calculateShare should be an abstract function like _getVaultSharePrice, that is implemented in the specific adaptors.

Handle

cmichel

Vulnerability details

Vulnerability Details

The protocol computes a factor when minting (and burning) tokens which is the exchange rate of rebase to base tokens (base supply / total assets value), see GToken.factor(). The first user can manipulate this factor such that it always returns 0.

Example:

• Attacker deposits 100.0 DAI and mints 100 * 1e18 PWRD: DepositHandler.depositGToken with dollarAmount = 100.0 = 100 * 1e18, then ctrl.mintGToken(pwrd, msg.sender, 1e18) calls gt.mint(account, gt.factor(), amount=1e18) where gt.factor() returns getInitialBase() = 1e18 because the person is the first minter and it mints amount * factor / _BASE = 1e18
• The ctrl.mintGToken call also increases total assets: pnl.increaseGTokenLastAmount(...)
• The attacker now burns (withdraws) all minted tokens again except a single wei using one of the withdrawal functions in WithdrawHandler. Because of the withdrawal fee the total assets are only decreased by the post-fee amount (IPnL(pnl).decreaseGTokenLastAmount(pwrd, amount=userBalance - 1, bonus=fee);), i.e., with a 2% withdrawal fee the total assets stay at 2% of 100$ = 2 * 1e18.
• The result is that GToken.factor() always returns totalSupplyBase().mul(BASE).div(totalAssets) = 1 * 1e18 / (2 * 1e18) = 0

Impact

The resulting factor is 0 and thus any user deposits by depositGToken will mint 0 base tokens to the depositor. This means all deposits and future value accrues to the attacker who holds the only base tokens.

An attacker could even frontrun the first minter to steal their deposit this way.

Recommended Mitigation Steps

Uniswap solves a similar problem by sending the first 1000 tokens to the zero address which makes the attack 1000x more expensive. The same should work here, i.e., on first mint (total base supply == 0), lock some of the first minter's tokens by minting ~1% of the initial amount to the zero address instead of to the first minter.