Sushi Miso contest - cmichel's results

Handle

cmichel

Vulnerability details

The index of the item in the userItem.lockToItems[_tokenAddress] array is tracked in lockedItem[_id].userIndex (see lockTokens):

uint256 userIndex = userItem.lockToItems[_tokenAddress].length - 1;
lockedItem[_id].userIndex = userIndex;

However, when withdrawing tokens this link breaks, as the withdrawn item is removed (swapped with the last element in this array):

if(userItem.amount == 0) {
    uint256[] storage userItems = users[msg.sender].lockToItems[_tokenAddress];
    // @audit swaps last element (which stores item ID) with element at _index
    userItems[_index] = userItems[userItems.length -1];
    userItems.pop();
    // @audit swapping needs to change lockedItem[ userItems[_index] ].userIndex = index;
}

Impact

After withdrawing tokens, the swapped item's lockedItem[_id].userIndex is not the last element anymore, but takes the old item's _index now. The link is broken and getLockedItemAtId returns wrong data which, when used to get the index for withdrawTokens, will then refer to the wrong token and revert.

Recommended Mitigation Steps

After deleting the element in withdrawTokens, update the remaining swapped element's userIndex:

if(userItem.amount == 0) {
    uint256[] storage userItems = users[msg.sender].lockToItems[_tokenAddress];
    userItems[_index] = userItems[userItems.length -1];
    userItems.pop();
    
    // ADD THIS
    // userItems[_index] refers to the previously last item's ID
    lockedItem[ userItems[_index] ].userIndex = index;
}

Handle

cmichel

Vulnerability details

The address.transfer function is used to send ETH to an account. It is restricted to a low amount of GAS and might fail if GAS costs change in the future or if a smart contract's fallback function handler implements anything non-trivial.

Occurences:

• MISOFarmFactory.deployFarm
• MISOLauncher.deployLauncher
• MISOMarket.deployMarket
• MISOTokenFactory.deployToken
• ListFactory.deployPointList
• MISOAccessFactory.deployAccessControl
• CrowdSale.commitEth
• DutchAuction.commitEth
• HyperbolicAuction.commitEth
• MISORecipe0*.prepareMiso
• SafeTransfer._tokenPayment
• WETH9.withdraw

Recommended Mitigation Steps

Consider using the lower-level .call{value: value} instead and check it's success return value.

Handle

cmichel

Vulnerability details

The DutchAuction._currentPrice() is computed by multiplying with priceDrop(). However, priceDrop() already performs a division and the final current price, therefore, loses precision.

Note that priceDrop() could even return 0 for ultra-low prices or very long auctions.

Imagine the actual payment per auction token price is 10^-12 => startPrice and endPrice are set with 18 decimals as ~10^6, but for auctions over a year (31,536,000 seconds > 10^6) it'll then return 0.

Impact

Precision can be lost leading to less accurate token auction results or even completely breaking the auction if the price is very low and the auctions are very long.

Recommended Mitigation Steps

Perform all multiplications before divisions:

uint256 priceDiff = block.timestamp.sub(uint256(marketInfo.startTime)).mul(
    uint256(_marketPrice.startPrice.sub(_marketPrice.minimumPrice))
  ) / uint256(_marketInfo.endTime.sub(_marketInfo.startTime));

Handle

cmichel

Vulnerability details

Although the contract is supposed to accept LP tokens only, it does not restrict the tokens used and it could be that a fee-on-transfer token is used as the pool.lpToken.

Impact

This has led to many attacks, see here for write-ups. The gist is that the "pool" token balance in the contract can be reduced to a few wei by continuously depositing/withdrawing tokens due to accounting errors with not using pre-post transfer balances to get the actual amount. (Attacker deposits 1000, is credited 1000, but pool actually only receives 900. Attacker can withdraw 1000, pool balance diff is -100.) Then pool.accRewardsPerShare = pool.accRewardsPerShare.add(rewardsAccum.mul(1e12).div(lpSupply)); is a really high value and it's multiplied by the wrong user.amount.

Recommended Mitigation Steps

Document that pool.lpToken should never be a fee-on-transfer token.

Use pre-and-post balances to compute the actual deposited amount.

Handle

cmichel

Vulnerability details

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 MISORecipe03.createLiquidityFarm uses a standard ERC20(rewardToken).transferFrom(msg.sender, self, tokensToFarm) call for the rewardToken which could be a non-compliant token like USDT

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in this recipe 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

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Calls to .approve with user-defined tokens are made in:

• MISOLauncher.createLauncher
• MISOMarket.createMarket

Impact

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

The DutchAuction.clearingPrice function can save gas by caching the computed prices instead of recomputing it.

Recommended Mitigation Steps

Cache the values:

function clearingPrice() public view returns (uint256) {
    /// @dev If auction successful, return tokenPrice
    uint256 _tokenPrice = tokenPrice();
    uint256 _currentPrice = priceFunction();
    return _tokenPrice > _currentPrice ? _tokenPrice : _currentPrice;
}

Handle

cmichel

Vulnerability details

The SushiToken.delegateBySig function can save gas by not taking the unnecessary nonce parameter. It currently validates that the provided nonce == sigNonces[signatory] but this is unnecessary as the nonce is already part of the signed message digest.

A shorter function payload leads to gas improvements.

Recommended Mitigation Steps

Remove the nonce parameter from the function, and use the sigNonces[signatory] when creating the message digest.

bytes32 structHash = keccak256(
    abi.encode(
        DELEGATION_TYPEHASH,
        delegatee,
        // @audit make sure to increment here when removing the require check below
        sigNonces[signatory]++,
        nonce,
        expiry
    )
);