Yield micro contest #1 - cmichel's results

Handle

cmichel

Vulnerability details

The CompositeMultiOracle.peek/get functions seem to return wrong prices. It's unclear what decimals source.decimals refers to in this case. Does it refer to source.source token decimals?

It chains the price arguments through _peek function calls and a single price is computed as:

(priceOut, updateTimeOut) = IOracle(source.source).peek(base, quote, 10 ** source.decimals);   // Get price for one unit
// @audit shouldn't this divide by 10 ** IOracle(source.source).decimals() instead?
priceOut = priceIn * priceOut / (10 ** source.decimals);

Assume all oracles use 18 decimals (oracle.decimals() returns 18) and source.decimals refers to the token decimals of source.source.

Then going from USDC -> DAI -> USDT (path = [DAI]) starts with a price of 1e18 in peek:

• _peek(USDC, DAI, 1e18): Gets the price of 1e6 USDC (as USDC has 6 decimals) in DAI with 18 decimals precision (because all oracle precision is set to 18): priceOut = priceIn * 1e18 / 1e6 = 1e18 * 1e18 / 1e6 = 1e30
• _peek(DAI, USDT, 1e30): Gets the price of 1e18 DAI (DAI has 18 decimals) with 18 decimals precision: priceOut = priceIn * 1e18 / 1e18 = priceIn = 1e30

It then uses 1e30 as the price to go from USDC to USDT: value = price * amount / 1e18 = 1e30 * (1.0 USDC) / 1e18 = 1e30 * 1e6 / 1e18 = 1e18 = 1e12 * 1e6 = 1_000_000_000_000.0 USDT. Inflating the actual USDT amount.

Recommended Mitigation Steps

The issue is that peek assumes that the final price is in 18 decimals in the value = price * amount / 1e18 division by 1e18. But _peek (and _get) don't enforce this.

_peek should scale the prices to 1e18 by doing:

(priceOut, updateTimeOut) = IOracle(source.source).get(base, quote, 10 ** source.decimals);
// priceOut will have same decimals as priceIn if we divide by oracle decimals
priceOut = priceIn * priceOut / (10 ** IOracle(source.source).decimals());

It does not need to divide by the source.source token precision (source.decimals), but by the oracle precision (IOracle(source.source).decimals()).

Handle

cmichel

Vulnerability details

The ERC20Rewards._updateRewardsPerToken function exits without updating rewardsPerToken_.lastUpdated if totalSupply is zero, i.e., if there are no tokens initially.

This leads to an error if there is an active rewards period but not tokens have been minted yet.

Example: rewardsPeriod.start: 1 month ago, rewardsPeriod.end: in 1 month, totalSupply == 0. The first mint leads to the user (mintee) receiving all rewards for the past period (50% of the total rewards in this case).

• _mint is called, calls _updateRewardsPerToken which short-circuits. rewardsPerToken.lastUpdated is still set to rewardsPeriod.start from the constructor. Then _updateUserRewards is called and does not currently yield any rewards. (because both balance and the index diff are zero). User is now minted the tokens, totalSupply increases and user balance is set.
• User performs a claim: _updateRewardsPerToken is called and timeSinceLastUpdated = end - rewardsPerToken_.lastUpdated = block.timestamp - rewardsPeriod.start = 1 month. Contract "issues" rewards for the past month. The first mintee receives all of it.

Impact

The first mintee receives all pending rewards when they should not receive any past rewards. This can easily happen if the token is new, the reward period has already been initialized and is running, but the protocol has not officially launched yet. Note that setRewards also allows setting a date in the past which would also be fatal in this case.

Recommended Mitigation Steps

The rewardsPerToken_.lastUpdated field must always be updated in _updateRewardsPerToken to the current time (or end) even if _totalSupply == 0. Don't return early.

Handle

cmichel

Vulnerability details

The setRewards function allows setting a different token. Holders of a previous reward period cannot all be paid out and will receive their old reward amount in the new token.

This leads to issues when the new token is more (less) valuable, or uses different decimals.

Example: Assume the first reward period paid out in DAI which has 18 decimals. Someone would have received 1.0 DAI = 1e18 DAI if they called claim now. Instead, they wait until the new period starts with USDC (using only 6 decimals) and can claim their 1e18 reward amount in USDC which would equal 1e12 USDC, one trillion USD.

Impact

Changing the reward token only works if old and new tokens use the same decimals and have the exact same value. Otherwise, users that claim too late/early will lose out.

Recommended Mitigation Steps

Disallow changing the reward token, or clear user's pending rewards of the old token. The second approach requires more code changes and keeping track of what token a user last claimed.

Handle

cmichel

Vulnerability details

The claim function performs an ERC20 transfer rewardsToken.transfer(to, claiming); but does not check the return value, nor does it work with all legacy tokens.

Some tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

The TimeLock.schedule function reverts if the same targets and data fields are used as the txHash will be the same. This means one cannot schedule the same transactions multiple times.

Impact

Imagine the delay is set to 30 days, but a contractor needs to be paid every 2 weeks. One needs to wait 30 days before scheduling the second payment to them.

Recommended Mitigation Steps

Also include eta in the hash. (Compound's Timelock does it as well.) This way the same transaction data can be used by specifying a different eta.