Platform: Code4rena
Start Date: 25/11/2021
Pot Size: $80,000 USDC
Total HM: 35
Participants: 32
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 27
Id: 59
League: ETH
Rank: 12/32
Findings: 3
Award: $1,748.26
🌟 Selected for report: 5
🚀 Solo Findings: 0
🌟 Selected for report: thank_you
Also found by: 0x0x0x, Koustre, Meta0xNull, WatchPug, cmichel, defsec, harleythedog, hyh, leastwood, pauliax, pmerkleplant, tabish, xYrYuYx
defsec
Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price. The attacker's profit is the protocol's loss.
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 and sellMalt functions:
https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/DexHandlers/UniswapHandler.sol#L148 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/DexHandlers/UniswapHandler.sol#L173
router.swapExactTokensForTokens( rewardBalance, 0, // amountOutMin path, address(this), now );
None
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 such that they are not visible in the public mempool.
#0 - 0xScotch
2021-12-10T00:17:20Z
#219
#1 - GalloDaSballo
2022-01-09T22:14:36Z
Duplicate of #219
🌟 Selected for report: cmichel
Also found by: 0x1f8b, Meta0xNull, WatchPug, defsec, jayjonah8, leastwood, stonesandtrees
defsec
All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/ERC20VestedMine.sol#L18 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/Malt.sol#L22 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/RewardSystem/RewardOverflowPool.sol#L17 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/TransferService.sol#L21 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/DAO.sol#L30 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/RewardReinvestor.sol#L38 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/SwingTrader.sol#L30
Manual Code Review
While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.
#0 - 0xScotch
2021-12-08T13:03:34Z
#245
#1 - GalloDaSballo
2022-01-24T01:22:20Z
Duplicate of #245
🌟 Selected for report: defsec
367.919 USDC - $367.92
defsec
During the code review, It has been seen that malt price return value has not been checked on the function. If oracle is returned price as a 0, fullReturn will be zero on the earlyExitReturn function.
Code Review
Consider to add return value check. The maltPrice should be more than zero for the calculation.
""" require(dexHandler.maltMarketPrice()>0, "Price should be more than zero"); """
#0 - GalloDaSballo
2022-01-24T01:17:40Z
Agree with the finding, alternatively there could be a revert in maltMarketPrice to ensure that 0 is never returned
🌟 Selected for report: defsec
367.919 USDC - $367.92
defsec
SafeMath library functions are not always used in arithmetic operations in the contracts, which could potentially cause integer underflow/overflows. Although in the reference lines of code, there are upper limits on the variables to ensure an integer underflow/overflow could not happen, using SafeMath is always a best practice, which prevents underflow/overflows completely (even if there were no assumptions on the variables) and increases code consistency as well.
https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/Auction.sol#L795 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/Auction.sol#L821 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/AuctionBurnReserveSkew.sol#L64 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/AuctionEscapeHatch.sol#L76 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/AuctionEscapeHatch.sol#L188
Code Review
Consider using the SafeMath library functions in the referenced lines of code.
#0 - GalloDaSballo
2022-01-24T01:19:16Z
Agree with the finding, while this is not indicative of any vulnerability, the finding is useful, and highlights a potential inconsistency in the sponsors code
🌟 Selected for report: defsec
367.919 USDC - $367.92
defsec
An overflow/underflow happens when an arithmetic operation reaches the maximum or minimum size of a type. For instance if a number is stored in the uint8 type, it means that the number is stored in a 8 bits unsigned number ranging from 0 to 2^8-1. In computer programming, an integer overflow occurs when an arithmetic operation attempts to create a numeric value that is outside of the range that can be represented with a given number of bits – either larger than the maximum or lower than the minimum representable value.
On the SwingTrader.sol contract, Some of the internal variables doesn't have overflow protection although the safemath library has been used.
function buyMalt(uint256 maxCapital) external onlyRole(STABILIZER_NODE_ROLE, "Must have stabilizer node privs") returns (uint256 capitalUsed) { if (maxCapital == 0) { return 0; } uint256 balance = collateralToken.balanceOf(address(this)); if (balance == 0) { return 0; } if (maxCapital < balance) { balance = maxCapital; } collateralToken.safeTransfer(address(dexHandler), balance); dexHandler.buyMalt(); deployedCapital = deployedCapital + balance; return balance; }
None
Consider to use the following statement.
deployedCapital = deployedCapital.add(balance);
#0 - 0xScotch
2021-12-06T13:42:04Z
If deployedCapital here overflows a uint256 then there are many other issues for the blockchain as the DAI supply has overflowed
#1 - GalloDaSballo
2022-01-24T01:26:37Z
The sponsor acknowledges, but in their reply they show that they don't need the overflow check. I'd recommend the sponsor to add a comment on the decision and be done with it. Will mark as valid, but if the sponsor disputed i would have sided with them
defsec
The following contract functions performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.
https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/DexHandlers/UniswapHandler.sol#L167 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/AuctionParticipant.sol#L59 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/StabilizerNode.sol#L252 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/RewardReinvestor.sol#L107
None
Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.
#0 - 0xScotch
2021-12-08T15:59:12Z
#247
#1 - GalloDaSballo
2022-01-24T01:21:34Z
Duplicate of #247
🌟 Selected for report: defsec
367.919 USDC - $367.92
defsec
After pragma version, 0.7.0, the contract should use block.timestamp.
None
It is recommended to use block.timestamp instead of now.
#0 - GalloDaSballo
2022-01-24T01:27:53Z
Finding is valid in a vacuum, but sponsors code is 0.6.6. (see hardhat config). We'll allow, but believe it's a nofix for the sponsor
🌟 Selected for report: 0x0x0x
Also found by: defsec, pmerkleplant, ye0lde
10.3016 USDC - $10.30
defsec
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L219 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L265 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L385 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L639 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L663 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L861 https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Auction.sol#L972
Code Review
Use "!=0" instead of ">0" for the gas optimization.
#0 - 0xScotch
2021-12-08T18:27:31Z
#271
10.3016 USDC - $10.30
defsec
++i is more gas efficient than i++ in loops forwarding.
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/AuctionBurnReserveSkew.sol#L54"
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MovingAverage.sol#L60"
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MovingAverage.sol#L187"
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MovingAverage.sol#L226"
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MovingAverage.sol#L292"
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MovingAverage.sol#L329"
"https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/MovingAverage.sol#L431"
Code Review
It is recommend to use unchecked{++i} and change i declaration to uint256.
#0 - 0xScotch
2021-12-09T22:04:13Z
#295
#1 - GalloDaSballo
2021-12-31T19:56:00Z
Duplicate of #295
15.2616 USDC - $15.26
defsec
Lower than uint256 size storage instance variables are actually less gas efficient. E.g. using uint8 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.
https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/RewardSystem/RewardDistributor.sol#L121 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/RewardSystem/RewardDistributor.sol#L126
None
Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with 8.
#0 - 0xScotch
2021-12-09T22:38:48Z
#289
#1 - GalloDaSballo
2022-01-07T00:39:28Z
Duplicate of #289
99.3381 USDC - $99.34
defsec
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
IERC20(token).safeApprove(address(operator), 0); IERC20(token).safeApprove(address(operator), amount);
https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/DexHandlers/UniswapHandler.sol#L167 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/AuctionParticipant.sol#L59 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/StabilizerNode.sol#L252 https://github.com/code-423n4/2021-11-malt/blob/d3f6a57ba6694b47389b16d9d0a36a956c5e6a94/src/contracts/RewardReinvestor.sol#L107
None
Approve with a zero amount first before setting the actual amount.
#0 - GalloDaSballo
2022-01-18T14:50:06Z
Agree with the finding, to avoid reverts you want to safeApprove(0) first to ensure allowance always goes from zero to non-zero