Malt Finance contest - harleythedog's results

Handle

harleythedog

Vulnerability details

Impact

The buyMalt and sellMalt functions in the UniswapHandler.sol do not have any slippage protection, which make them vulnerable to a sandwich attack. An attacker can strategically manipulate a uniswap pool before these functions are called, forcing the contract to buy/sell at an inflated price. The attacker can then undo their manipulation and end up with more tokens than they started with. This is a common issue and has been marked as a medium finding on previous C4 reports.

The fix for this is to set the amountOutMin parameter correctly in the uniswap calls, instead of just leaving it as 0.

Proof of Concept

buyMalt: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L131

sellMalt: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L160

Also, see M-01 on this previous C4 report - it is the exact same issue: https://ipfs.io/ipfs/bafybeicjla2h26q3wz4s344bsrtvhkxr3ypm44owvrzyorb2t6tcptlmem/C4%20Slingshot%20report.pdf

Tools Used

Inspection

Recommended Mitigation Steps

Add slippage protection to mititgate sandwich attack thefts

Handle

harleythedog

Vulnerability details

Impact

There is an admin function setRewardToken in AbstractRewardMine.sol that allows changing the reward token of the contract. If there are unclaimed rewards in the contract when this function is called, then users should be transferred these rewards first before the new reward token is set. If this doesn't happen, then the users will miss out on their reward since the last time they called withdrawAll. If the users have a lot of unclaimed reward, the worse this problem is.

Proof of Concept

See setRewardToken here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/AbstractRewardMine.sol#L273

Notice that there is no consideration for the currently unclaimed rewards in the contract.

Tools Used

Inspection

Recommended Mitigation Steps

Add logic to setRewardToken to allow users to claim their owed old reward tokens before the new reward token is set.

Handle

harleythedog

Vulnerability details

Impact

In AuctionParticipant.sol, the function setReplenishingIndex is an admin function that allows manually setting replenishingIndex. As I have shown in my two previous findings, I believe that this function could be called frequently. In my opinion (and Murphy's law would agree), this implies that eventually an admin will accidentally set replenishingIndex incorrectly with this function.

Right now, setReplenishingIndex does not allow the admin to set replenishingIndex to a value smaller than it currently is. So, if an admin were to accidentally set this value too high, then it would be impossible to set it back to a lower value (the higher the value set, the worse this issue). All of the unclaimed tokens on auctions at smaller indices would be locked forever.

Proof of Concept

See code for setReplenishingIndex here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/AuctionParticipant.sol#L132

Tools Used

Inspection

Recommended Mitigation Steps

Remove the require statement on line 136, so that an admin can set the index to a smaller value.

Handle

harleythedog

Vulnerability details

Impact

There are several ERC20 tokens that take a small fee on transfers/transferFroms (known as "fee-on-transfer" tokens). Most notably, USDT is an ERC20 token that has togglable transfer fees, but for now the fee is set to 0 (see the contract here: https://etherscan.io/address/0xdAC17F958D2ee523a2206206994597C13D831ec7#code). For these tokens, it should not be assumed that if you transfer x tokens to an address, that the address actually receives x tokens. In the current test environment, DAI is the only collateralToken available, so there are no issues. However, it has been noted that more pools will be added in the future, so special care will need to be taken if fee-on-transfer tokens (like USDT) are planned to be used as collateralTokens.

For example, consider the function purchaseArbitrageTokens in Auction.sol. This function transfers realCommitment amount of collateralToken to the liquidityExtension, and then calls purchaseAndBurn(realCommitment) on the liquidityExtension. The very first line of purchaseAndBurn(amount) is require(collateralToken.balanceOf(address(this)) >= amount, "Insufficient balance");. In the case of fee-on-transfer tokens, this line will revert due to the small fee taken. This means that all calls to purchaseArbitrageTokens will fail, which would be very bad when the price goes below peg, since no one would be able to participate in this auction.

Proof of Concept

See purchaseArbitrageTokens here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/Auction.sol#L177

See purchaseAndBurn here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/LiquidityExtension.sol#L117

Tools Used

Inspection

Recommended Mitigation Steps

Add logic to transfers/transferFroms to calculate exactly how many tokens were actually sent to a specific address. In the example given with purchaseArbitrageTokens, instead of calling purchaseAndBurn with realCommitment, the contract should use the difference in the liquidityExtension balance after the transfer minus the liquidityExtension balance before the transfer.

Handle

harleythedog

Vulnerability details

Impact

In AuctionParticpant.sol, every time purchaseArbitrageTokens is called, the current auction is pushed to auctionIds. If this function were to be called on the same auction multiple times, then the same auction id would be pushed multiple times into this array, and the claim function would have issues with replenishingIndex.

Specifically, even if replenishingIndex was incremented once in claim, it is still possible that the auction at the next index will never reward any more tokens to the participant, so the contract would need manual intervention to set replenishingIndex (due to the if statement on lines 79-82 that does nothing if there is no claimable yield).

It is likely that purchaseArbitrageTokens would be called multiple times on the same auction. In fact, the commented out code for handleDeficit (in ImpliedCollateralService.sol) even suggests that the purchases might happen within the same transaction. So this issue will likely be an issue on most auctions and would require manual setting of replenishingIndex.

NOTE: This is a separate issue from the one I just submitted previously relating to replenishingIndex. The previous issue was related to an edge case where replenishingIndex might need to be incremented by one if there are never going to be more claims, while this issue is due to duplicate auction ids.

Proof of Concept

See code for purchaseArbitrageTokens here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/AuctionParticipant.sol#L40

Notice that currentAuction is always appended to auctionIds.

Tools Used

Inspection

Recommended Mitigation Steps

Add a check to the function to purchaseArbitrageTokens to ensure that duplicate ids are not added. For example, this can be achieved by changing auctionIds to a mapping instead of an array.

Handle

harleythedog

Vulnerability details

Impact

In Bonding.sol, the internal function _unbondAndBreak transfers a user's stake tokens to the dexHandler and then calls removeLiquidity on the dexHandler. Within the Uniswap handler (which is the only handler so far) removeLiquidity takes special care in the edge case where router.removeLiquidity returns zero tokens. Specifically, the Uniswap handler has this code:

if (amountMalt == 0 || amountReward == 0) {
	liquidityBalance = lpToken.balanceOf(address(this));
	lpToken.safeTransfer(msg.sender, liquidityBalance);
	return (amountMalt, amountReward);
}

If this edge case does indeed happen (i.e. if something is preventing the Uniswap router from removing liquidity at the moment), then the Uniswap handler will transfer the LP tokens back to Bonding.sol. However, Bonding.sol does not have any logic to recognize that this happened, so the LP tokens will become stuck in the contract and the user will never get any of their value back. This could be very bad if the user unbonds a lot of LP and they don't get any of it back.

Proof of Concept

See _unbondAndBreak here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/Bonding.sol#L226

Notice how the edge case where amountMalt == 0 || amountReward == 0 is not considered in this function, but it is considered in the Uniswap handler's removeLiquidity here: https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L240

Tools Used

Inspection.

Recommended Mitigation Steps

Add a similar edge case check to _unbondAndBreak. In the case where LP tokens are transferred back to Bonding.sol instead of malt/reward, these LP tokens should be forwarded back to the user since the value is rightfully theirs.