OlympusTokenMigrator Manual Review Findings

OTM-01M: Improper Integration w/ Uniswap V2

Description:

The way the migration of the token works can cause the migration to either completely halt or cause the liquidity position to significantly diminish in value should the governor address be a contract that can be actuated from anyone, such as a Timelock forked from Compound.

Example:

Copy
249/**
250 *   @notice Migrate LP and pair with new OHM
251 */
252function migrateLP(
253    address pair,
254    bool sushi,
255    address token
256) external onlyGovernor {
257    uint256 oldLPAmount = IERC20(pair).balanceOf(address(oldTreasury));
258    oldTreasury.manage(pair, oldLPAmount);
259
260    IUniswapV2Router router = sushiRouter;
261    if (!sushi) {
262        router = uniRouter;
263    }
264
265    IERC20(pair).approve(address(router), oldLPAmount);
266    (uint256 amountA, uint256 amountB) = router.removeLiquidity(
267        token, 
268        address(oldOHM), 
269        oldLPAmount,
270        0, 
271        0, 
272        address(this), 
273        1000000000000
274    );
275
276    newTreasury.mint(address(this), amountB);
277
278    IERC20(token).approve(address(router), amountA);
279    newOHM.approve(address(router), amountB);
280
281    router.addLiquidity(
282        token, 
283        address(newOHM), 
284        amountA, 
285        amountB, 
286        amountA, 
287        amountB, 
288        address(newTreasury), 
289        100000000000
290    );
291}

Recommendation:

We strongly recommend the migration procedure to be revised. In the current state, it specifies the expected output amounts as 0 which can cause an arbitreur to significantly skew the pair, diminish the LP position one-sidedly (i.e. towards OHM) and cause the liquidity removal to be in the native token only. This can cause the protocol to crash due to the artificial inflation of OHM's price which can be performed with the help of flash loans if for example the governor is a Timelock implementation relying on a GovernorAlpha to actuate it. Additionally, the liquidity provision is also performed incorrectly as it specifies the amounts that should at minimum be set within the pair to be equal to the amounts provided. This case is only true when the pair has not been created before. Should newOHM units circulate in the market before this point, it would be possible for someone to race the transaction, create the pair with miniscule amounts and thus cause the migration to be impossible. As a last note, the current block.timestamp can and should be passed in as the expiry argument instead of the literal 1000000000000 which is meaningless.

Alleviation:

Minimum arguments were properly added to the migrateLP function and the numeric literal was substituted for the current block.timestamp, thereby alleviating this exhibit in full.

OTM-02M: Improper Evaluation of Token Balance

Description:

The _migrateToken function is utilizing the tokenValue yielded by the newTreasury implementation yet is comparing it with the excessReserves of the oldTreasury.

Example:

Copy
357/**
358 *   @notice Migrate token from old treasury to new treasury
359 */
360function _migrateToken(address token, bool deposit) internal {
361    uint256 balance = IERC20(token).balanceOf(address(oldTreasury));
362
363    uint256 excessReserves = oldTreasury.excessReserves();
364    uint256 tokenValue = newTreasury.tokenValue(token, balance);
365
366    if (tokenValue > excessReserves) {
367        tokenValue = excessReserves;
368        balance = excessReserves * 10**9;
369    }
370
371    oldTreasury.manage(token, balance);
372
373    if (deposit) {
374        IERC20(token).safeApprove(address(newTreasury), balance);
375        newTreasury.deposit(balance, token, tokenValue);
376    } else {
377        IERC20(token).transfer(address(newTreasury), balance);
378    }
379}

Recommendation:

We strongly recommend the valueOf implementation of the legacy treasury to be utilized instead as it is currently incorrectly evaluating the maximum value that can be retrieved from oldTreasury.

Alleviation:

The code now properly uttilizes the legacy valueOf function to properly identify how many funds can be managed from the legacy treasury.

OTM-03M: Ungraceful Mint Handling

Description:

The contract contains logic blocks that indicate the possibility of oldOHM being minted beyond migration is realistic and simply prohibits swaps for it, however, a mint event and corresponding transfer to the migrator contract is unaccounted for and can cause defund to be inoperable.

Example:

Copy
208// withdraw backing of migrated OHM
209function defund(address reserve) external onlyGovernor {
210    require(ohmMigrated && timelockEnd < block.number && timelockEnd != 0);
211    oldwsOHM.unwrap(oldwsOHM.balanceOf(address(this)));
212
213    uint256 amountToUnstake = oldsOHM.balanceOf(address(this));
214    oldsOHM.approve(address(oldStaking), amountToUnstake);
215    oldStaking.unstake(amountToUnstake, false);
216
217    uint256 balance = oldOHM.balanceOf(address(this));
218
219    oldSupply = oldSupply.sub(balance);
220
221    uint256 amountToWithdraw = balance.mul(1e9);
222    oldOHM.approve(address(oldTreasury), amountToWithdraw);
223    oldTreasury.withdraw(amountToWithdraw, reserve);
224    IERC20(reserve).safeTransfer(address(newTreasury), IERC20(reserve).balanceOf(address(this)));
225
226    emit Defunded(balance);
227}

Recommendation:

We strongly recommend the code to gracefully handle such an event by containing an if block that nullifies oldSupply if the balance exceeds it.

Alleviation:

The code now properly handles an instance of the balance exceeding the oldSupply in accordance to our recommendation.

OTM-04M: Potential of Repeat Invocation

Description:

The startTimelock function can be invoked multiple times.

Example:

Copy
229// start timelock to send backing to new treasury
230function startTimelock() external onlyGovernor {
231    timelockEnd = block.number.add(timelockLength);
232
233    emit TimelockStarted(block.number, timelockEnd);
234}

Recommendation:

It should only be invoke-able once and as such should introduce a require check that ensures timelockEnd is equal to 0 at the beginning.

Alleviation:

The require check we recommended was properly added to the codebase.