Malt Finance contest - cmichel's results

Handle

cmichel

Vulnerability details

The contracts are missing slippage checks which can lead to being vulnerable to sandwich attacks.

A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.

See UniswapHandler.buyMalt:

router.swapExactTokensForTokens(
  rewardBalance,
  // @audit allows any out amount, can trade at bad price
  0, // amountOutMin
  path,
  address(this),
  now
);

Impact

Trades can happen at a bad price and lead to receiving fewer Malt tokens than at a fair market price. The attacker's profit is the protocol's loss.

Recommended Mitigation Steps

Add minimum return amount checks. Accept a function parameter that can be chosen by the transaction sender, then check that the actually received amount is above this parameter.

Alternatively, check if it's feasible to send these transactions directly to a miner (flashbots) such that they are not visible in the public mempool.

Handle

cmichel

Vulnerability details

The MovingAverage.sol contract defines several variables that in the end make the samples array act as a ring buffer:

• sampleMemory: The total length (buffer size) of the samples array. samples is initialized with sampleMemory zero observations.
• counter: The pending sample index (modulo sampleMemory)

The _getFirstSample function computes the first sample as (counter + 1) % sampleMemory which returns the correct index only if the ring buffer is full, i.e., it wraps around. (in the counter + 1 >= sampleMemory).

If the samples array does not wrap around yet, the zero index should be returned instead.

Impact

Returning counter + 1 if counter + 1 < sampleMemory returns a zero initialized samples observation index. This then leads to a wrong computation of the TWAP.

Recommended Mitigation Steps

Add an additional check for if (counter + 1 < sampleMemory) return 0 in _getFirstSample.

Handle

cmichel

Vulnerability details

The UniswapHandler.maltMarketPrice function returns a tuple of the price and the decimals of the price. However, the returned decimals do not match the computed price for the else if (rewardDecimals < maltDecimals) branch:

else if (rewardDecimals < maltDecimals) {
  uint256 diff = maltDecimals - rewardDecimals;
  price = (rewardReserves.mul(10**diff)).mul(10**rewardDecimals).div(maltReserves);
  decimals = maltDecimals;
}

Note that rewardReserves are in reward token decimals, maltReserves is a malt balance amount (18 decimals). Then, the returned amount is in rewardDecimals + diffDecimals + rewardDecimals - maltDecimals = maltDecimals + rewardDecimals - maltDecimals = rewardDecimals. However decimals = maltDecimals is wrongly returned.

Impact

Callers to this function will receive a price in unexpected decimals and might inflate or deflate the actual amount. Luckily, the AuctionEscapeHatch decides to completely ignore the returned decimals and as all prices are effectively in rewardDecimals, even if stated in maltDecimals, it currently does not seem to lead to an issue.

Recommendation

Fix the function by returning rewardDecimals instead of maltDecimals in the rewardDecimals < maltDecimals branch.

Handle

cmichel

Vulnerability details

The Permissions.reassignGlobalAdmin function is supposed to only be run with the TIMELOCK_ROLE role, see onlyRole(TIMELOCK_ROLE, "Only timelock can assign roles").

However, the TIMELOCK_ROLE is not the admin of all the reassigned roles and the revokeRole(role, oldAccount) calls will fail as it requires the ADMIN_ROLE.

Recommended Mitigation Steps

The idea might have been that only the TIMELOCK should be able to call this function, and usually it is also an admin, but the function strictly does not work if the caller only has the TIMELOCK roll and will revert in this case. Maybe governance decided to remove the admin role from the Timelock, which makes it impossible to call reassignGlobalAdmin anymore as both the timelock and admin are locked out.

Handle

cmichel

Vulnerability details

Certain ERC20 tokens make modifications to their ERC20's transfer or balanceOf functions. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Impact

The Bonding._bond() function will revert in the _balanceCheck when transferring a fee-on-transfer token as it assumes the entire amount was received.

Recommended Mitigation Steps

To support fee-on-transfer tokens, measure the asset change right before and after the asset-transferring calls and use the difference as the actual bonded amount.

Handle

cmichel

Vulnerability details

When adding liquidity through UniswapHandler.addLiquidity, the entire contract balances are used to add liquidity and the min amounts are set to 95% of these balances. If the balances in this contract are unbalanced (the ratio is not similar to the current Uniswap pool reserve ratios) then this function will revert and no liquidity is added.

See UniswapHandler.buyMalt:

(maltUsed, rewardUsed, liquidityCreated) = router.addLiquidity(
  address(malt),
  address(rewardToken),
  maltBalance, // @audit-info amountADesired
  rewardBalance,
  // @audit assumes that whatever is in this contract is already balanced. good assumption?
  maltBalance.mul(95).div(100), // @audit-info amountAMin
  rewardBalance.mul(95).div(100),
  msg.sender, // transfer LP tokens to sender
  now
);

Impact

If the contract has unbalanced balances, then the router.addLiquidity call will revert. Note that an attacker could even send tokens to this contract to make them unbalanced and revert, resulting in a griefing attack.

Recommended Mitigation Steps

It needs to be ensured that the balances in the contract are always balanced and match the current reserve ratio. It might be better to avoid directly using the balances which can be manipulated by transferring tokens to the contract and accepting parameters instead of how many tokens to provide liquidity with from the caller side.