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: 6/32
Findings: 6
Award: $5,166.49
🌟 Selected for report: 6
🚀 Solo Findings: 1
0x0x0x
Delay can be changed without any delay. Therefore, it is possible to call functions from this contract in a single block by changing the delay. This creates a huge attack vector, since if governor private keys would be stolen, everything can be withdrawn in several blocks. Even, if the private keys won't be stolen, users can not do anything against a rug pull. Effectively, this contract doesn't provide the functionality it should.
In OZ timelock contracts, for setDelay function, there is a requirement that msg.sender == address(this). By doing so changing the delay is also timelocked, so it can not be abused to do a rug pull in a single block. Add this statement to setDelay to make sure it will function properly.
#0 - 0xScotch
2021-12-08T12:47:48Z
#263
#1 - GalloDaSballo
2022-01-08T17:10:55Z
Duplicate of #263
🌟 Selected for report: 0x0x0x
3679.1897 USDC - $3,679.19
0x0x0x
AuctionEschapeHatch.sol#exitEarly takes as input amount to represent how much of the
When the user exits an auction with profit, to apply the profit penalty less maltQuantity is liquidated compared to how much malt token the liquidated amount corresponds to. The problem is auction.amendAccountParticipation() simply subtracts the malt quantity with penalty and full amount from users auction stats. This causes a major problem, since in _calculateMaltRequiredForExit those values are used for calculation by calculating maltQuantity as follow:
uint256 maltQuantity = userMaltPurchased.mul(amount).div(userCommitment);
The ratio of userMaltPurchased / userCommitment gets higher after each profit taking (since penalty is applied to substracted maltQuantity from userMaltPurchased), by doing so a user can earn more than it should. Since after each profit taking users commitment corresponds to proportionally more malt, the user can even reduce profit penalties by dividing exitEarly calls in several calls.
In other words, the ratio of userMaltPurchased / userCommitment gets higher after each profit taking and user can claim more malt with less commitment. Furthermore after all userMaltPurchased is claimed the user can have userCommitment left over, which can be used to claimArbitrage, when possible.
Make sure which values are used for what and update values which doesn't create problems like this. Rethink about how to track values of an auction correctly.
#0 - GalloDaSballo
2022-01-25T02:08:13Z
The warden has identified an exploit that allows early withdrawers to gain more rewards than expected. Anytime "points" and rewards need to be earned over time, it's ideal to accrue points in order to distribute them (see how Compound or AAVE tokens work) Because the warden showed a flow in the accounting logic for the protocol, I agree with high severity.
🌟 Selected for report: thank_you
Also found by: 0x0x0x, Koustre, Meta0xNull, WatchPug, cmichel, defsec, harleythedog, hyh, leastwood, pauliax, pmerkleplant, tabish, xYrYuYx
0x0x0x
In UniswapHandler.sol two important functions sellMalt and buyMalt use swapExactTokensForTokens with amountOutMin = 0. This is a big problem since miners can exploit this intensively. So miners can strongly manipulate the price, since they can order the sell and buy commands arbitrarily and there is no further check avoiding big manipulations. Even the oracles use moving average, this can be exploited by only accepting sell or buy orders in a block.
As a consequence price can heavily get manipulated and users and protocol can get scammed.
At least approximately provide a amountOutMin
#0 - 0xScotch
2021-12-10T00:22:34Z
#219
#1 - GalloDaSballo
2022-01-09T22:13:22Z
Duplicate of #219
🌟 Selected for report: 0x0x0x
Also found by: harleythedog, hyh
298.0144 USDC - $298.01
0x0x0x
In case the reward token is changed, totalDeclaredReward will be changed and likely equal to 0. Since _userStakePadding and _globalStakePadding are accumulated, changing the reward token will not reset those values. Thus, it will create problems.
I think it would be the best to remove this function.
If you want to keep it, then it must have an event and it should be used by a timelock contract. Furthermore, it has to be used carefully and the new token should be distributed such that padding variables still make sense.
#0 - GalloDaSballo
2022-01-09T22:38:51Z
Agree with highlighting the Admin Privilege, however because this is contingent on a malicious Admin, I'll downgrade the finding to Medium Severity.
Mitigation could be done by ensuring old rewards are sent out, still claimable, or by making the rewardToken immutable
🌟 Selected for report: pmerkleplant
Also found by: 0x0x0x, GiveMeTestEther, WatchPug, pauliax, robee, ye0lde
0x0x0x
Example:
for (uint i = 0; i < arr.length; i++) { //Operations not effecting the length of the array. }
Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Furthermore, there is no need to assign the initial value 0. This costs extra gas. Moreover, i = i + 1 costs more compared to i++ .
Recommended implementation:
uint length = arr.length; for (uint i; i < length; i++) { //Operations not effecting the length of the array. }
By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).
./Auction.sol:407: for (uint i = 0; i < epochCommitments.length; ++i) { ./AuctionParticipant.sol:107: for (uint256 i = replenishingIndex; i < auctionIds.length; i = i + 1) { ./DexHandlers/UniswapHandler.sol:315: for (uint i = 0; i < buyers.length - 1; i = i + 1) { ./Malt.sol:34: for (uint256 i = 0; i < minters.length; i = i + 1) { ./Malt.sol:37: for (uint256 i = 0; i < burners.length; i = i + 1) { ./MiningService.sol:48: for (uint i = 0; i < mines.length; i = i + 1) { ./MiningService.sol:69: for (uint i = 0; i < mines.length; i = i + 1) { ./MiningService.sol:86: for (uint i = 0; i < mines.length; i = i + 1) { ./MiningService.sol:96: for (uint i = 0; i < mines.length; i = i + 1) { ./MiningService.sol:142: for (uint i = 0; i < mines.length - 1; i = i + 1) { ./MiningService.sol:166: for (uint i = 0; i < mines.length; i = i + 1) { ./TransferService.sol:87: for (uint i = 0; i < verifierList.length - 1; i = i + 1) { ./libraries/UniswapV2Library.sol:66: for (uint i; i < path.length - 1; i++) { ./libraries/UniswapV2Library.sol:77: for (uint i = path.length - 1; i > 0; i--) {
#0 - 0xScotch
2021-12-09T22:02:25Z
#366
#1 - GalloDaSballo
2022-01-18T13:23:37Z
Duplicate of #106
🌟 Selected for report: 0x0x0x
Also found by: defsec, pmerkleplant, ye0lde
10.3016 USDC - $10.30
0x0x0x
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
./AbstractRewardMine.sol:147: if (rewardDenominator > 0) { ./Auction.sol:219: require(amountTokens > 0, "No claimable Arb tokens"); ./Auction.sol:265: return auction.endingTime > 0 && (now >= auction.endingTime || auction.finalPrice > 0 || auction.commitments >= auction.maxCommitments); ./Auction.sol:385: return auction.startingTime > 0; ./Auction.sol:639: if (realBurnBudget > 0) { ./Auction.sol:659: require(auction.startingTime > 0, "No auction available for the given id"); ./Auction.sol:663: if (auction.maltPurchased > 0) { ./Auction.sol:861: if (auction.commitments > 0 || !auction.finalized) { ./Auction.sol:894: require(_length > 0, "Length must be larger than 0"); ./Auction.sol:972: require(_split > 0 && _split <= 10000, "Must be between 0-100%"); ./Auction.sol:980: require(_maxEnd > 0 && _maxEnd <= 1000, "Must be between 0-100%"); ./Auction.sol:988: require(_lookback > 0, "Must be above 0"); ./Auction.sol:996: require(_lookback > 0, "Must be above 0"); ./Auction.sol:1004: require(_bps > 0 && _bps < 1000, "Must be between 0-100%"); ./Auction.sol:1012: require(_threshold > 0, "Must be between greater than 0"); ./AuctionBurnReserveSkew.sol:109: if (aggregate.maxCommitments > 0) { ./AuctionBurnReserveSkew.sol:190: require(_lookback > 0, "Cannot have zero lookback period"); ./AuctionEscapeHatch.sol:191: require(amount > 0, "Nothing to claim"); ./AuctionEscapeHatch.sol:222: require(_earlyExitBps > 0 && _earlyExitBps <= 1000, "Must be between 0-100%"); ./AuctionEscapeHatch.sol:230: require(_period > 0, "Cannot have 0 lookback period"); ./AuctionPool.sol:118: if (globalRewarded > 0 && userReward > 0) { ./AuctionPool.sol:125: if (forfeitAmount > 0) { ./AuctionPool.sol:129: if (declaredRewardDecrease > 0) { ./AuctionPool.sol:141: if (forfeitedRewards > 0) { ./Bonding.sol:87: require(amount > 0, "Cannot bond 0"); ./Bonding.sol:97: require(amount > 0, "Cannot unbond 0"); ./Bonding.sol:101: require(bondedBalance > 0, "< bonded balance"); ./Bonding.sol:117: require(amount > 0, "Cannot unbond 0"); ./Bonding.sol:121: require(bondedBalance > 0, "< bonded balance"); ./Bonding.sol:283: if (diff > 0) { ./DAO.sol:47: if (offeringMint > 0) { ./DAO.sol:78: require(amount > 0, "Cannot have zero amount"); ./DAO.sol:94: require(_length > 0, "Cannot have zero length epochs"); ./ERC20VestedMine.sol:93: if (globalRewarded > 0 && userReward > 0) { ./ERC20VestedMine.sol:115: if (forfeitReward > 0) { ./ERC20VestedMine.sol:119: if (declaredRewardDecrease > 0) { ./ForfeitHandler.sol:49: if (swingTraderCut > 0) { ./ForfeitHandler.sol:53: if (treasuryCut > 0) { ./ImpliedCollateralService.sol:64: if (maxAmount > 0) { ./ImpliedCollateralService.sol:68: if (maxAmount > 0) { ./ImpliedCollateralService.sol:71: // if (maxAmount > 0) { ./ImpliedCollateralService.sol:74: // if (maxAmount > 0) { ./LiquidityExtension.sol:161: require(_ratio > 0 && _ratio <= 100, "Must be between 0 and 100"); ./MaltDataLab.sol:234: require(_price > 0, "Cannot have 0 price"); ./MaltDataLab.sol:242: require(_lookback > 0, "Cannot have 0 lookback"); ./MaltDataLab.sol:250: require(_lookback > 0, "Cannot have 0 lookback"); ./MaltDataLab.sol:258: require(_lookback > 0, "Cannot have 0 lookback"); ./MovingAverage.sol:385: if (oldSample.timestamp > 0 && activeSamples > 1) { ./MovingAverage.sol:412: require(_sampleLength > 0, "Cannot have 0 second sample length"); ./MovingAverage.sol:428: require(_sampleMemory > 0, "Cannot have sample memroy of 0"); ./PoolTransferVerification.sol:76: require(newThreshold > 0 && newThreshold < 10000, "Threshold must be between 0-100%"); ./PoolTransferVerification.sol:85: require(lookback > 0, "Cannot have 0 lookback"); ./RewardReinvestor.sol:93: require(rewardLiquidity > 0, "Cannot reinvest 0"); ./RewardReinvestor.sol:115: if (maltBalance > 0) { ./RewardReinvestor.sol:119: if (rewardTokenBalance > 0) { ./RewardSystem/RewardDistributor.sol:144: require(reward > 0, "Cannot declare 0 reward"); ./RewardSystem/RewardDistributor.sol:266: if (amount > 0) { ./RewardSystem/RewardDistributor.sol:277: if (amount > 0) { ./RewardSystem/RewardOverflowPool.sol:76: require(_maxFulfillment > 0, "Can't have 0 max fulfillment"); ./RewardSystem/RewardThrottle.sol:85: if (aprTarget > 0 && _epochAprGivenReward(epoch, balance) > aprTarget) { ./RewardSystem/RewardThrottle.sol:89: if (remainder > 0) { ./RewardSystem/RewardThrottle.sol:271: if (underflow > 0) { ./RewardSystem/RewardThrottle.sol:323: require(_smoothingPeriod > 0, "No zero smoothing period"); ./StabilizerNode.sol:270: if (callerCut > 0) { ./StabilizerNode.sol:274: if (auctionPoolCut > 0) { ./StabilizerNode.sol:278: if (swingTraderCut > 0) { ./StabilizerNode.sol:282: if (treasuryCut > 0) { ./StabilizerNode.sol:286: if (daoCut > 0) { ./StabilizerNode.sol:290: if (lpCut > 0) { ./StabilizerNode.sol:359: require(_period > 0, "Must be greater than 0"); ./StabilizerNode.sol:406: require(_incentive > 0, "No negative incentive"); ./StabilizerNode.sol:417: require(amount > 0, "No negative damping"); ./StabilizerNode.sol:449: require(_upper > 0 && _lower > 0, "Must be above 0"); ./StabilizerNode.sol:488: require(_maxContribution > 0 && _maxContribution <= 100, "Must be between 0 and 100"); ./StabilizerNode.sol:552: require(_period > 0, "Cannot have 0 period"); ./StabilizerNode.sol:561: require(_distance > 0 && _distance < 1000, "Override must be between 0-100%"); ./StabilizerNode.sol:570: require(_period > 0, "Cannot have 0 period"); ./SwingTrader.sol:136: if (profit > 0) { ./libraries/SafeBurnMintableERC20.sol:70: if (returndata.length > 0) { // Return data is optional ./libraries/UniswapV2Library.sol:37: require(amountA > 0, 'UniswapV2Library: INSUFFICIENT_AMOUNT'); ./libraries/UniswapV2Library.sol:38: require(reserveA > 0 && reserveB > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY'); ./libraries/UniswapV2Library.sol:44: require(amountIn > 0, 'UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT'); ./libraries/UniswapV2Library.sol:45: require(reserveIn > 0 && reserveOut > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY'); ./libraries/UniswapV2Library.sol:54: require(amountOut > 0, 'UniswapV2Library: INSUFFICIENT_OUTPUT_AMOUNT'); ./libraries/UniswapV2Library.sol:55: require(reserveIn > 0 && reserveOut > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY'); ./libraries/UniswapV2Library.sol:77: for (uint i = path.length - 1; i > 0; i--) {
#0 - GalloDaSballo
2021-12-28T13:56:38Z
Finding is valid
#1 - GalloDaSballo
2021-12-31T17:00:39Z
is a single operation
GT!= is two operationsNOT``EQ
0x0x0x
If a uint is checked, whether it is bigger equal to 0, this statement always hold and it is a waste of gas.
./SwingTrader.sol:170: require(_profitCut >= 0 && _profitCut <= 1000, "Must be between 0 and 100%"); ./Timelock.sol:71: _delay >= 0 && _delay < gracePeriod,
#0 - 0xScotch
2021-12-08T18:16:39Z
#309
#1 - GalloDaSballo
2021-12-30T16:15:17Z
Duplicate of #309
15.2616 USDC - $15.26
0x0x0x
This function can be defined external rather than public. Thus, it will save gas.
#0 - 0xScotch
2021-12-09T22:41:02Z
#163
15.2616 USDC - $15.26
0x0x0x
In MovingAverage.sol, uint64 is used for computation of time etc. But computations with uint64 does cost more gas and furthermore block.timestamp is uint256, which is additionally casted to uint64. uint32 is used for indexes, but this can also be changed with uint256.
Same applies for RewardDistributer.sol.
Use uint256 rather than custom uint.
#0 - GalloDaSballo
2022-01-07T00:38:44Z
This brought up some interesting conversation on twitter https://twitter.com/_hrkrshnn/status/1477682676853133315
The short answer is that there is no advantage in using smaller ints, and in some cases it ends up being more expensive.
🌟 Selected for report: 0x0x0x
56.5245 USDC - $56.52
0x0x0x
uint256 progressionBps = (block.timestamp - auctionEndTime) * 10000 / cooloffPeriod; if (progressionBps > 10000) { progressionBps = 10000; } if (fullReturn > amount) { // Allow a % of profit to be realised uint256 maxProfit = (fullReturn - amount) * (maxEarlyExitBps * progressionBps / 10000) / 1000; uint256 desiredReturn = amount + maxProfit; maltQuantity = desiredReturn.mul(pegPrice) / currentPrice; } return maltQuantity;
progressionBps is only used, if there is a profit. Calculations of this parameter should be under if statement checking whether there is a profit to save gas and increase readability as follows:
if (fullReturn > amount) { // Allow a % of profit to be realised uint256 progressionBps = (block.timestamp - auctionEndTime) * 10000 / cooloffPeriod; if (progressionBps > 10000) { progressionBps = 10000; } uint256 maxProfit = (fullReturn - amount) * (maxEarlyExitBps * progressionBps / 10000) / 1000; uint256 desiredReturn = amount + maxProfit; maltQuantity = desiredReturn.mul(pegPrice) / currentPrice; } return maltQuantity;
#0 - GalloDaSballo
2022-01-18T13:21:30Z
Agree with the finding, limiting the calculation to the then statement avoids the needles computation in the else case
🌟 Selected for report: 0x0x0x
56.5245 USDC - $56.52
0x0x0x
AbstractRewardMine.sol#_removeFromStakePadding takes as input a reason, but as reason always < stake padding is used. Thus, by hard coding it gas can be saved.
#0 - GalloDaSballo
2022-01-18T13:22:48Z
Am not sure on the exact gas savings of removing a parameter (am assuming cost of passing those variables) but I agree with the finding
🌟 Selected for report: ye0lde
Also found by: 0x0x0x, GiveMeTestEther, WatchPug
10.3016 USDC - $10.30
0x0x0x
When a variable is declared solidity assigns the default value. In case the contract assigns the value again, it costs extra gas.
Example:uint x = 0 costs more gas than uint x without having any different functionality.
./Auction.sol:351: uint256 secondsSinceStart = 0; ./Auction.sol:626: uint256 premiumExcess = 0; ./AuctionBurnReserveSkew.sol:92: uint256 initialAuction = 0; ./AuctionBurnReserveSkew.sol:108: uint256 participation = 0; ./AuctionBurnReserveSkew.sol:117: uint256 initialIndex = 0; ./AuctionBurnReserveSkew.sol:124: uint256 total = 0; ./AuctionPool.sol:115: uint256 earnedReward = 0; ./DAO.sol:19: uint256 public epoch = 0; ./ERC20VestedMine.sol:90: uint256 earnedReward = 0; ./FaucetTwo.sol:24: uint256 balance = 0; ./RewardSystem/RewardDistributor.sol:94: uint256 vestedReward = 0; ./RewardSystem/RewardThrottle.sol:125: uint256 totalAPR = 0; ./RewardSystem/RewardThrottle.sol:149: uint256 totalProfit = 0; ./RewardSystem/RewardThrottle.sol:150: uint256 totalBondedValue = 0; ./SwingTrader.sol:112: uint256 profit = 0;
#0 - 0xScotch
2021-12-09T22:28:42Z
#80
#1 - GalloDaSballo
2021-12-27T22:35:35Z
Duplicate of #80