Malt Finance contest - 0x1f8b's results

Handle

0x1f8b

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The buyMalt function is intended to only be called by BUYER_ROLE, this function extracts the balance from rewardToken and performs a swap to send the malt obtained to the sender. On the other hand, the sellMalt function can be invoked by anyone, as long as they have a malt balance, they will perform the reverse step, sell the malt for rewardToken, and send it to the sender. (https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L181)

The problem appears on the line, https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L181, since the sender does not receive the balance swap, he receive the total that the contract has, so if the contract had 1,000,000 DAI pending to be called in buyMalt, with a frontRunning attack, you can send 1 MALT to the contract, and call sellMalt, it would transfer to the attacker the entire balance of DAIs pending purchase.

Tools Used

Manual review

Recommended Mitigation Steps

Use rights for sell, or use transferFrom and allowance

Handle

0x1f8b

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

It is a common practice in all the revised contracts not to check the inputs of the initialize methods, the addresses are not checked that they are different from address(0), some values of ranges are not checked that they are in the range that they have to be , and this can ruin the entire contract deployment because they can only be initialized once.

It is convenient to validate all inputs and constructors or initializers are not exempt from this recommendation.

One example it's:

• https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MaltDataLab.sol#L54 price target could be zero, and produce a denial of service with div(priceTarget) but in https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/MaltDataLab.sol#L234 it's checked the right value.

Tools Used

Manual review

Recommended Mitigation Steps

Check inputs in all initialize

Handle

0x1f8b

Vulnerability details

Impact

A malicious attacker could take control of the contract and mint from an unwanted account.

Proof of Concept

As can be seen in the explorer, https://mumbai.polygonscan.com/txs?a=0xee94d4eafe86cb5b3989e655ea26253f9f844c77&p=8 between the deploy and the initialize there is a difference of 46 blocks, which gives plenty of time to send a initialize itself and perform a front running attack.

https://mumbai.polygonscan.com/tx/0xef485fa5829832e6dcec9f431941afd3f2bd07930f3c6a6efd06334170c01053

However, the front running attack in the initialize is usually easily detected since the owner variables are overwritten and it can be easily verified that the owner of the contract has been impersonating, the important thing in this case is that if the pool is listened to and the original values ​​are copied, but by adding an entry to the minters argument, the owner of the contract will see that apparently all the values ​​have been initialized as desired, because the sender is not used anywhere moment, so the attacker will have access to mint as much Malt as he wishes, by being registered with the role of MONETARY_MINTER_ROLE.

https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/Malt.sol#L26

Tools Used

Manual review

Recommended Mitigation Steps

Set the owner to msg.sender, and check it in every steps

Handle

0x1f8b

Vulnerability details

Impact

Cheaper storage.

Proof of Concept

The struct AuctionData file Auction.sol is optimizable. It looks like this:

struct AuctionData {
  // The full amount of commitments required to return to peg
  uint256 fullRequirement;
  // total maximum desired commitments to this auction
  uint256 maxCommitments;
  // Quantity of sale currency committed to this auction
  uint256 commitments;
  // Malt purchased and burned using current commitments
  uint256 maltPurchased;
  // Desired starting price for the auction
  uint256 startingPrice;
  // Desired lowest price for the arbitrage token
  uint256 endingPrice;
  // Price of arbitrage tokens at conclusion of auction. This is either
  // when the duration elapses or the maxCommitments is reached
  uint256 finalPrice;
  // The peg price for the liquidity pool
  uint256 pegPrice;
  // Time when auction started
  uint256 startingTime;
  uint256 endingTime;
  // Is the auction currently accepting commitments?
  bool active;
  // The reserve ratio at the start of the auction
  uint256 preAuctionReserveRatio;
  // Has this auction been finalized? Meaning any additional stabilizing
  // has been done
  bool finalized;
  // The amount of arb tokens that have been executed and are now claimable
  uint256 claimableTokens;
  // The finally calculated realBurnBudget
  uint256 finalBurnBudget;
  // The amount of Malt purchased with realBurnBudget
  uint256 finalPurchased;
  // A map of all commitments to this auction by specific accounts
  mapping(address => AccountCommitment) accountCommitments;
}

But active and finalized, the unique boolean values, should be together, otherwise they will spend two slots instead of one.

  uint256 preAuctionReserveRatio;
  bool active;
  bool finalized;

Tools Used

Manual review

Recommended Mitigation Steps