Omniscia Olympus DAO Audit

OlympusTokenMigrator Code Style Findings

OlympusTokenMigrator Code Style Findings

OTM-01C: Inexistent Error Messages

Description:

The linked require checks have no explicit error messages defined.

Example:

contracts/migration/OlympusTokenMigrator.sol
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 advise them to be set so to aid in the validation of the conditionals as well as in debugging the application.

Alleviation:

Error messages were included to all require checks across the contract.

OTM-02C: Multiple Top-Level Declarations

TypeSeverityLocation
Code StyleInformationalOlympusTokenMigrator.sol:L17-L44, L46-L50

Description:

The linked interface implementations should be relocated to dedicated files to not pollute the top-level of the contract file.

Example:

contracts/migration/OlympusTokenMigrator.sol
17interface IUniswapV2Router {
18    function addLiquidity(
19        address tokenA,
20        address tokenB,
21        uint256 amountADesired,
22        uint256 amountBDesired,
23        uint256 amountAMin,
24        uint256 amountBMin,
25        address to,
26        uint256 deadline
27    )
28        external
29        returns (
30            uint256 amountA,
31            uint256 amountB,
32            uint256 liquidity
33        );
34

35    function removeLiquidity(
36        address tokenA,
37        address tokenB,
38        uint256 liquidity,
39        uint256 amountAMin,
40        uint256 amountBMin,
41        address to,
42        uint256 deadline
43    ) external returns (uint256 amountA, uint256 amountB);
44}
45

46interface IStakingV1 {
47    function unstake(uint256 _amount, bool _trigger) external;
48

49    function index() external view returns (uint256);
50}
51

52contract OlympusTokenMigrator is OlympusAccessControlled {

Recommendation:

We advise them to be relocated to the interfaces sub-folder, potentially under an external second-level subfolder, to ensure that the code structure of the system is maintainable.

Alleviation:

All required interfaces were split to their dedicated files and are now properly imported to the codebase.

OTM-03C: Redundant & Confusing Comparisons

TypeSeverityLocation
Gas OptimizationInformationalOlympusTokenMigrator.sol:L137, L182

Description:

The if-else structure within migrate and _send evaluate all states of the enum redundantly which can also cause ambiguous behaviour if the compiler does not enforce the value range of the TYPE enum due to a compiler issue given that it is internally represented by a uint8.

Example:

contracts/migration/OlympusTokenMigrator.sol
118enum TYPE {
119    UNSTAKED,
120    STAKED,
121    WRAPPED
122}
123

124// migrate OHMv1, sOHMv1, or wsOHM for OHMv2, sOHMv2, or gOHM
125function migrate(
126    uint256 _amount,
127    TYPE _from,
128    TYPE _to
129) external {
130    uint256 sAmount = _amount;
131    uint256 wAmount = oldwsOHM.sOHMTowOHM(_amount);
132

133    if (_from == TYPE.UNSTAKED) {
134        oldOHM.safeTransferFrom(msg.sender, address(this), _amount);
135    } else if (_from == TYPE.STAKED) {
136        oldsOHM.safeTransferFrom(msg.sender, address(this), _amount);
137    } else if (_from == TYPE.WRAPPED) {
138        oldwsOHM.safeTransferFrom(msg.sender, address(this), _amount);
139        wAmount = _amount;
140        sAmount = oldwsOHM.wOHMTosOHM(_amount);
141    }
142

143    if (ohmMigrated) {
144        require(oldSupply >= oldOHM.totalSupply(), "OHMv1 minted");
145        _send(wAmount, _to);
146    } else {
147        gOHM.mint(msg.sender, wAmount);
148    }
149}

Recommendation:

We advise the last else if branch to be converted to an else branch to ensure transfer of funds is performed at all times from the user to the contract and vice versa for the migration to occur.

Alleviation:

The if-else optimization was only applied to the first linked segment thereby partially alleviating this exhibit.

Prev

OlympusERC20.sol (OER-C)

Next

Ownable.sol (OWN-C)

On this page