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: 1/32
Findings: 7
Award: $8,396.38
🌟 Selected for report: 23
🚀 Solo Findings: 2
993.3812 USDC - $993.38
WatchPug
The purpose of a Timelock contract is to put a limit on the privileges of the governor, by forcing a two step process with a preset delay time.
However, we found that the current implementation actually won't serve that purpose as it allows the governor to execute any transactions without any constraints.
To do that, the current governor can call Timelock#setGovernor(address _governor) and set a new governor effective immediately.
And the new governor can then call Timelock#setDelay() and change the delay to 0, also effective immediately.
The new governor can now use all the privileges without a delay, including granting minter role to any address and mint unlimited amount of MALT.
In conclusion, a Timelock contract is supposed to guard the protocol from lost private key or malicious actions. The current implementation won't fulfill that mission.
function setGovernor(address _governor) public onlyRole(GOVERNOR_ROLE, "Must have timelock role") { _swapRole(_governor, governor, GOVERNOR_ROLE); governor = _governor; emit NewGovernor(_governor); }
function setDelay(uint256 _delay) public onlyRole(GOVERNOR_ROLE, "Must have timelock role") { require( _delay >= 0 && _delay < gracePeriod, "Timelock::setDelay: Delay must not be greater equal to zero and less than gracePeriod" ); delay = _delay; emit NewDelay(delay); }
Consider making setGovernor and setDelay only callable from the Timelock contract itself.
Specificaly, changing from onlyRole(GOVERNOR_ROLE, "Must have timelock role") to require(msg.sender == address(this), "...").
Also, consider changing _adminSetup(_admin) in Timelock#initialize() to _adminSetup(address(this)), so that all roles are managed by the timelock itself as well.
#0 - GalloDaSballo
2022-01-08T17:10:36Z
The warden has identified an exploit that allows to sidestep the delay for the timelock, effectively bypassing all of the timelock's security guarantees. Because of the gravity of this, I agree with the high risk severity.
Mitigation can be achieved by ensuring that all operations run under a time delay
🌟 Selected for report: WatchPug
3679.1897 USDC - $3,679.19
WatchPug
function getPegDeltaFrequency() public view returns (uint256) { uint256 initialIndex = 0; uint256 index; if (count > auctionAverageLookback) { initialIndex = count - auctionAverageLookback; } uint256 total = 0; for (uint256 i = initialIndex; i < count; ++i) { index = _getIndexOfObservation(i); total = total + pegObservations[index]; } return total * 10000 / auctionAverageLookback; }
When count < auctionAverageLookback, at L131, it should be return total * 10000 / count;. The current implementation will return a smaller value than expected.
The result of getPegDeltaFrequency() will be used for calculating realBurnBudget for auctions. With the result of getPegDeltaFrequency() being inaccurate, can result in an improper amount of excess Liquidity Extension balance to be used at the end of an auction.
#0 - 0xScotch
2021-12-03T16:52:57Z
I actually think this should be higher severity. This bug could manifest in liquidity extension being depleted to zero which could have catastrophic consequences downstream.
#1 - GalloDaSballo
2022-01-23T01:43:23Z
Agree with the finding, this is an incorrect logic in the protocol, which can limit it's functionality and as the sponsor says: could have catastrophic consequences downstream as such I'll increase the severity to high.
Mitigation seems to be straightforward
🌟 Selected for report: thank_you
Also found by: 0x0x0x, Koustre, Meta0xNull, WatchPug, cmichel, defsec, harleythedog, hyh, leastwood, pauliax, pmerkleplant, tabish, xYrYuYx
WatchPug
function splitReinvest(uint256 rewardLiquidity) external { _retrieveReward(rewardLiquidity); uint256 rewardBalance = rewardToken.balanceOf(address(this)); rewardToken.safeTransfer(address(dexHandler), rewardBalance.div(2)); dexHandler.buyMalt(); _bondAccount(msg.sender); emit SplitReinvest(msg.sender, rewardLiquidity); }
The current implementation of splitReinvest() provides no parameter for slippage control, making them vulnerable to front-run attacks.
splitReinvest();Consider adding a amountOutMin parameter.
#0 - 0xScotch
2021-12-10T00:22:39Z
#219
#1 - GalloDaSballo
2022-01-09T23:58:35Z
Duplicate of #219
🌟 Selected for report: WatchPug
1103.7569 USDC - $1,103.76
WatchPug
function setSampleMemory(uint256 _sampleMemory) external onlyRole(ADMIN_ROLE, "Must have admin privs") { require(_sampleMemory > 0, "Cannot have sample memroy of 0"); if (_sampleMemory > sampleMemory) { for (uint i = sampleMemory; i < _sampleMemory; i++) { samples.push(); } counter = counter % _sampleMemory; } else { activeSamples = _sampleMemory; // TODO handle when list is smaller Tue 21 Sep 2021 22:29:41 BST } sampleMemory = _sampleMemory; }
In the current implementation, when sampleMemory is updated, the samples index will be malposition, making getValueWithLookback() get the wrong samples, so that returns the wrong value.
10movingAverage.update(1e18) being called for 120 timesmovingAverage.setSampleMemory(118) and set sampleMemory to 118The current movingAverage.getValueWithLookback(sampleLength * 10) returns 0.00000203312 e18, while it's expeceted to be 1e18
After setSampleMemory(), getValueWithLookback() may also return 0or revert FullMath: FULLDIV_OVERFLOW at L134.
Consider removing setSampleMemory function.
#0 - GalloDaSballo
2022-01-10T00:51:49Z
I agree that calling setSampleMemory will cause issues, and can cause opportunities to further extract value.
However this can be triggered by an admin action.
I'll think about the severity, but as of now, because it is contingent on admin privilege, will downgrade to Medium
#1 - GalloDaSballo
2022-01-30T16:38:46Z
I stand by my decision of Medium Severity. While the consequences can be troublesome, they are contingent on the admin breaking / griefing the system
WatchPug
When the price of Malt is off the lowerThreshold and upperThreshold, StabilizerNode.sol will market buy/sell Malt.
However, since the market sell can be triggered by anyone, and there is no slippage control, it makes it vulnerable to flashloan sandwich attack.
function stabilize() external notSameBlock { auction.checkAuctionFinalization(); require( block.timestamp >= stabilizeWindowEnd || _stabilityWindowOverride(), "Can't call stabilize" ); stabilizeWindowEnd = block.timestamp + stabilizeBackoffPeriod; rewardThrottle.checkRewardUnderflow(); uint256 exchangeRate = maltDataLab.maltPriceAverage(priceAveragePeriod); if (!_shouldAdjustSupply(exchangeRate)) { maltDataLab.trackReserveRatio(); lastStabilize = block.timestamp; return; } emit Stabilize(block.timestamp, exchangeRate); if (exchangeRate > maltDataLab.priceTarget()) { _distributeSupply(); } else { _startAuction(); } lastStabilize = block.timestamp; }
function _distributeSupply() internal { ... uint256 swingAmount = swingTrader.sellMalt(tradeSize); ... malt.mint(address(dexHandler), tradeSize); emit MintMalt(tradeSize); uint256 rewards = dexHandler.sellMalt();
function _startAuction() internal { ... uint256 amountUsed = swingTrader.buyMalt(purchaseAmount);
When the price is below lowerThreshold.
The attacker can:
stabilize(), the swingTrader will buy Malt at a higher price;Consider making stabilize() function only allowed to be called by an EOA to prevent flash loan sandwich attack.
#0 - 0xScotch
2021-12-03T12:55:18Z
This bug needs to be mitigated. However, the POC example given doesn't apply as under lowerThreshold would put the protocol into recovery mode where non-whitelisted buyers would be blocked.
We can add a before transfer hook on Malt that will trigger stabilize when a Malt sell is going through above upperThreshold. This will protect against the frontrunning above peg.
Guarding stabilize to only EOA is a good idea though.
#1 - 0xScotch
2021-12-08T12:27:51Z
#56
#2 - GalloDaSballo
2022-01-09T23:54:20Z
@0xScotch I have yet to judge this finding.
However be aware that a Only-EOA check (msg.sender != tx.origin) is destined to eventually fail as eventually EIP-3074 will make it so that there's no difference between a contract and an EOA
#3 - bitdeep
2022-01-12T19:33:23Z
Proposing onlyEOA via contract size check:
function isContract(address account) public view returns (bool) { uint size; assembly { size := extcodesize(account) } return size > 0; }
#4 - GalloDaSballo
2022-01-25T02:16:44Z
Proposing onlyEOA via contract size check:
function isContract(address account) public view returns (bool) { uint size; assembly { size := extcodesize(account) } return size > 0; }
@bitdeep I believe this has been proven wrong multiple times.
Basicaly isContract (the check you're doing) works only for verifying that something is a contract.
It doesn't guarantee that something is not a contract.
That's because a contract can be written to perform operation in it's constructor, while in it's constructor, the check for size := extcodesize(account) will return 0 (because space was not yet allocated)
In summary, you can always verify if something IS a contract. And for now you can still use the msg.sender == tx.origin check, but eventually you will not have any way of proving that something is an EOA
#5 - GalloDaSballo
2022-01-25T02:25:00Z
From reading the code I believe that anyone can sell MALT at any time.
This means that the token can be sold with the goal of reducing it's price.
After the price reduction a call to stabilize can be performed.
This allows the caller to get a small caller incentive as well as to trigger an auction.
The auction effectively triggers the system to sell the min(idealAmount, currenrtlyOwnerAmoutn) with the goal of bringing back the malt price to peg.
The question that this left unsolved is how would the trader be able to get malt for a discount to then dump it for again.
I think this can be merged as duplicate of #56
As such medium severity is more correct
36.2087 USDC - $36.21
WatchPug
function setMiningService(address _miningService) public onlyRole(ADMIN_ROLE, "Must have admin privs") { _swapRole(_miningService, miningService, MINING_SERVICE_ROLE); _swapRole(_miningService, miningService, REWARD_MANAGER_ROLE); miningService = _miningService; }
Considering that _miningService is a crucial settings, it's necessary to add require(_miningService != address(0), "...") to validate the input address.
Other examples include:
function bondToAccount(address account, uint256 amount) public { if (msg.sender != offering) { _notSameBlock(); } require(amount > 0, "Cannot bond 0"); miningService.onBond(account, amount); _bond(account, amount); }
accountfunction initialize( address _timelock, address initialAdmin, address _collateralToken, address _malt, address _dexHandler, address _stabilizerNode, address _rewardThrottle ) external initializer { _adminSetup(_timelock); _setupRole(ADMIN_ROLE, initialAdmin); _setupRole(STABILIZER_NODE_ROLE, _stabilizerNode); collateralToken = ERC20(_collateralToken); malt = ERC20(_malt); dexHandler = IDexHandler(_dexHandler); rewardThrottle = IRewardThrottle(_rewardThrottle); }
_timelockinitialAdmin_dexHandlerfunction initialize( address _timelock, address initialAdmin, address _rewardToken, address _treasuryMultisig, address _swingTrader ) external initializer { _adminSetup(_timelock); _setupRole(ADMIN_ROLE, initialAdmin); rewardToken = ERC20(_rewardToken); treasuryMultisig = _treasuryMultisig; swingTrader = _swingTrader; }
_timelock_rewardToken_treasuryMultisig_swingTraderfunction initialize( address _timelock, address initialAdmin, address _rewardToken, address _malt, address _auction, address _uniswapV2Factory, address payable _treasuryMultisig, address _auctionPool ) external initializer { _adminSetup(_timelock); _setupRole(ADMIN_ROLE, initialAdmin); _setupRole(AUCTION_ROLE, _auction); rewardToken = ERC20(_rewardToken); malt = IBurnMintableERC20(_malt); auction = IAuction(_auction); uniswapV2Factory = _uniswapV2Factory; treasuryMultisig = _treasuryMultisig; auctionPool = _auctionPool; lastStabilize = block.timestamp; } function setupContracts( address _dexHandler, address _maltDataLab, address _auctionBurnReserveSkew, address _rewardThrottle, address _dao, address _swingTrader, address _liquidityExtension, address _impliedCollateralService ) external onlyRole(ADMIN_ROLE, "Must have admin role") { dexHandler = IDexHandler(_dexHandler); maltDataLab = IMaltDataLab(_maltDataLab); auctionBurnReserveSkew = IAuctionBurnReserveSkew(_auctionBurnReserveSkew); rewardThrottle = IRewardThrottle(_rewardThrottle); dao = IDAO(_dao); swingTrader = ISwingTrader(_swingTrader); liquidityExtension = ILiquidityExtension(_liquidityExtension); impliedCollateralService = IImpliedCollateralService(_impliedCollateralService); }
_timelockinitialAdmin_uniswapV2Factory_treasuryMultisig_daofunction initialize( address _timelock, address initialAdmin, uint256 _thresholdBps, address _maltDataLab, uint256 _lookback, address _pool ) external initializer { _adminSetup(_timelock); _setupRole(ADMIN_ROLE, initialAdmin); thresholdBps = _thresholdBps; maltDataLab = IMaltDataLab(_maltDataLab); priceLookback = _lookback; pool = _pool; }
_timelockinitialAdmin_poolfunction initialize( address _timelock, address initialAdmin ) external initializer { _adminSetup(_timelock); _setupRole(ADMIN_ROLE, initialAdmin); }
_timelockinitialAdmin#0 - GalloDaSballo
2022-01-24T01:46:35Z
Duplicate of #74
🌟 Selected for report: cmichel
Also found by: 0x1f8b, Meta0xNull, WatchPug, defsec, jayjonah8, leastwood, stonesandtrees
WatchPug
The initializer function that initializes important contract state can be called by anyone.
The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.
In the best case for the victim, they notice it and have to redeploy their contract costing gas.
Use the constructor to initialize non-proxied contracts.
For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.
#0 - 0xScotch
2021-12-08T13:03:42Z
#245
#1 - GalloDaSballo
2022-01-25T01:40:17Z
#245
165.5635 USDC - $165.56
WatchPug
The default upperStabilityThreshold and lowerStabilityThreshold assumes that rewardToken.decimals() is 18.
uint256 public upperStabilityThreshold = (10**18) / 100; // 1% uint256 public lowerStabilityThreshold = (10**18) / 100;
When the StabilizerNode.sol contract is initialized with a rewardToken with decimals of 8 (eg. USDC). upperThreshold and lowerThreshold will be much larger than expected.
function _shouldAdjustSupply(uint256 exchangeRate) internal view returns (bool) { uint256 decimals = rewardToken.decimals(); uint256 priceTarget = maltDataLab.priceTarget(); uint256 upperThreshold = priceTarget.mul(upperStabilityThreshold).div(10**decimals); uint256 lowerThreshold = priceTarget.mul(lowerStabilityThreshold).div(10**decimals);
Consider changing to:
uint256 public upperStabilityThresholdBps = 100; // 1% uint256 public lowerStabilityThresholdBps = 100;
function _shouldAdjustSupply(uint256 exchangeRate) internal view returns (bool) { uint256 decimals = rewardToken.decimals(); uint256 priceTarget = maltDataLab.priceTarget(); uint256 upperThreshold = priceTarget.mul(upperStabilityThreshold).div(10000); uint256 lowerThreshold = priceTarget.mul(lowerStabilityThreshold).div(10000);
#0 - GalloDaSballo
2022-01-23T01:41:01Z
upperStabilityThreshold and lowerStabilityThreshold are variables that can be changed by the admin, in their default value they imply a token with 18 decimals and if a different token where to be used the math would be off.
That is true.
A more elegant approach would be to recompute them if / when rewardToken is changed
rewardToken seems to be unchangeable.
Personally, I think the finding is valid, but the impact is extremely small, and the sponsor will be able to mitigate via a setter.
As such I'll downgrade to low
I think re-computing the variables based on the input token to be an ideal improvement for the code
WatchPug
There are many functions across the codebase that will perform 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.
Instances include:
auctionRewardToken.approve(address(auction), balance);
stakeToken.approve(address(bonding), liquidityCreated);
rewardToken.approve(address(auction), rewarded);
rewardToken.approve(address(router), rewardBalance);
lpToken.approve(address(router), liquidityBalance);
It is usually good to add a require-statement that checks the return value or to use something like safeApprove; unless one is sure the given token reverts in case of a failure.
#0 - 0xScotch
2021-12-08T15:59:03Z
#247
#1 - GalloDaSballo
2022-01-25T00:55:35Z
#247
🌟 Selected for report: WatchPug
367.919 USDC - $367.92
WatchPug
Based on the context, maltDataLab.trackReserveRatio() should be called once a market buy/sell is made.
However, in _distributeSupply() when swingAmount >= tradeSize, after a market sell, the function returned without maltDataLab.trackReserveRatio().
function stabilize() external notSameBlock { auction.checkAuctionFinalization(); require( block.timestamp >= stabilizeWindowEnd || _stabilityWindowOverride(), "Can't call stabilize" ); stabilizeWindowEnd = block.timestamp + stabilizeBackoffPeriod; rewardThrottle.checkRewardUnderflow(); uint256 exchangeRate = maltDataLab.maltPriceAverage(priceAveragePeriod); if (!_shouldAdjustSupply(exchangeRate)) { maltDataLab.trackReserveRatio(); lastStabilize = block.timestamp; return; } emit Stabilize(block.timestamp, exchangeRate); if (exchangeRate > maltDataLab.priceTarget()) { _distributeSupply(); } else { _startAuction(); } lastStabilize = block.timestamp; }
function _distributeSupply() internal { if (supplyDistributionController != address(0)) { bool success = ISupplyDistributionController(supplyDistributionController).check(); if (!success) { return; } } uint256 priceTarget = maltDataLab.priceTarget(); uint256 tradeSize = dexHandler.calculateMintingTradeSize(priceTarget).div(expansionDampingFactor); if (tradeSize == 0) { return; } uint256 swingAmount = swingTrader.sellMalt(tradeSize); // @Auditor: At this time, a market operation occurred, affecting the reserveRatio if (swingAmount >= tradeSize) { return; } tradeSize = tradeSize - swingAmount; malt.mint(address(dexHandler), tradeSize); emit MintMalt(tradeSize); uint256 rewards = dexHandler.sellMalt(); auctionBurnReserveSkew.addAbovePegObservation(tradeSize); uint256 remaining = _replenishLiquidityExtension(rewards); _distributeRewards(remaining); maltDataLab.trackReserveRatio(); impliedCollateralService.claim(); }
Consider moving maltDataLab.trackReserveRatio() from _distributeSupply(), _startAuction() to stabilize() before L173.
#0 - GalloDaSballo
2022-01-24T01:45:56Z
Finding is valid
🌟 Selected for report: WatchPug
367.919 USDC - $367.92
WatchPug
function provideReinvest(uint256 rewardLiquidity) external { _retrieveReward(rewardLiquidity); uint256 rewardBalance = rewardToken.balanceOf(address(this)); // This is how much malt is required uint256 maltLiquidity = dexHandler.getOptimalLiquidity(address(malt), address(rewardToken), rewardBalance); // Transfer the remaining Malt required malt.safeTransferFrom(msg.sender, address(this), maltLiquidity); _bondAccount(msg.sender); emit ProvideReinvest(msg.sender, rewardLiquidity); }
_retrieveReward will call MiningService.sol#withdrawRewardsForAccount() which uses amount as max withdrawnAmount, if there are no enough rewards, the actual rewarded amount will be less than amount.
function withdrawRewardsForAccount(address account, uint256 amount) public onlyRole(REINVESTOR_ROLE, "Must have reinvestor privs") { _withdrawMultiple(account, amount); } /* * INTERNAL FUNCTIONS */ function _withdrawMultiple(address account, uint256 amount) internal { for (uint i = 0; i < mines.length; i = i + 1) { if (!mineActive[mines[i]]) { continue; } uint256 withdrawnAmount = IRewardMine(mines[i]).withdrawForAccount(account, amount, msg.sender); amount = amount.sub(withdrawnAmount); if (amount == 0) { break; } } }
Consider using rewardBalance as the value of the reward parameter.
#0 - GalloDaSballo
2022-01-25T00:56:14Z
Agree with the finding
WatchPug
function setAuctionAverageLookback(uint256 _lookback) external onlyRole(ADMIN_ROLE, "Must have admin role") { require(_lookback > 0, "Cannot have zero lookback period"); if (_lookback > auctionAverageLookback) { for (uint i = auctionAverageLookback; i < _lookback; i++) { pegObservations.push(0); } } auctionAverageLookback = _lookback; emit SetAuctionAverageLookback(_lookback); }
function getPegDeltaFrequency() public view returns (uint256) { uint256 initialIndex = 0; uint256 index; if (count > auctionAverageLookback) { initialIndex = count - auctionAverageLookback; } uint256 total = 0; for (uint256 i = initialIndex; i < count; ++i) { index = _getIndexOfObservation(i); total = total + pegObservations[index]; } return total * 10000 / auctionAverageLookback; }
In the current implementation, when auctionAverageLookback is updated, the pegObservation index will be malposition, making getValueWithLookback() get the wrong samples, so that returns the wrong value.
addAbovePegObservation(amount) being called for 1000 times;setAuctionAverageLookback(1000) and set auctionAverageLookback to 1000Then, getPegDeltaFrequency() will return 0.01e4, while it's expected to return 1e4.
Consider removing setAuctionAverageLookback function.
#0 - 0xScotch
2021-12-08T12:20:05Z
#194
#1 - GalloDaSballo
2022-01-25T02:31:47Z
#194
🌟 Selected for report: WatchPug
367.919 USDC - $367.92
WatchPug
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; return amount + maxProfit; }
If we assume that maxEarlyExitBps is 200 and cooloffPeriod is 1 day, when progressionBps less than 50, (maxEarlyExitBps * progressionBps / 10000) will be 0 due to precision loss, which resulted in maxProfit is 0.
When maxEarlyExitBps is set smaller, the margin of error will be even larger.
Given:
purchaseArbitrageTokens() and purchase with 8,000 DAI;exitEarly(), it will mint 8,888.88 Malt and receive 8,000 DAI, while it's expected to 8,890 MALT and 8,000.96 DAI.Change to:
if (fullReturn > amount) { // Allow a % of profit to be realised uint256 maxProfit = (fullReturn - amount) * (maxEarlyExitBps * 1000 * progressionBps / 10000) / 1000 / 1000; return amount + maxProfit; }
#0 - GalloDaSballo
2022-01-26T13:37:09Z
I appreciate the updated math, and agree with the finding. I believe this to be a rounding error, as such I'll downgrade to Low Severity
🌟 Selected for report: WatchPug
Also found by: GiveMeTestEther, Meta0xNull, robee, ye0lde
7.4171 USDC - $7.42
WatchPug
Every reason string takes at least 32 bytes.
Use short reason strings that fits in 32 bytes or it will become more expensive.
Instances include:
function _notSameBlock() internal { require( block.number > lastBlock[_msgSender()], "Can't carry out actions in the same block" ); lastBlock[_msgSender()] = block.number; }
require(!auction.active, "Cannot claim tokens on an active auction");
require(redemption <= remaining.add(1), "Cannot claim more tokens than available");
require(auction.startingTime > 0, "No auction available for the given id");
require(!active, "Cannot exit early on an active auction");
require(_index > replenishingIndex, "Cannot replenishingIndex to old value");
require(block.timestamp >= getEpochStartTime(epoch + 1), "Cannot advance epoch until start of new epoch");
require(balance >= value, "ERC20Permit: transfer amount exceeds balance");
require(balance >= value, "ERC20Permit: transfer amount exceeds balance");
require(_service != address(0), "Cannot use address 0 as transfer service");
require(_sampleLength > 0, "Cannot have 0 second sample length");
require(forfeited <= _globals.declaredBalance, "Cannot forfeit more than declared");
require(amount <= _globals.declaredBalance, "Can't decrement more than total reward balance");
require( eta >= block.timestamp.add(delay), "Timelock::queueTransaction: Estimated execution block must satisfy delay." );
require( queuedTransactions[txHash], "Timelock::executeTransaction: Transaction hasn't been queued." ); require( block.timestamp >= eta, "Timelock::executeTransaction: Transaction hasn't surpassed time lock." ); require( block.timestamp <= eta.add(gracePeriod), "Timelock::executeTransaction: Transaction is stale." );
require( success, "Timelock::executeTransaction: Transaction execution reverted." );
#0 - GalloDaSballo
2021-12-27T22:13:55Z
Finding is Valid, most projects will setup a table of codes to be able to explain their errors, alternatively try to keep the strings short
Notice that the gas savings happen only on deployment as you're paying for cost of storing the string in the smart contracts bytecode
🌟 Selected for report: pmerkleplant
Also found by: 0x0x0x, GiveMeTestEther, WatchPug, pauliax, robee, ye0lde
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Instances include:
AuctionParticipant.sol#outstandingArbTokens()
MiningService.sol#balanceOfRewards()
TransferService.sol#removeVerifier()
Auction.sol#getAccountCommitments()
#0 - 0xScotch
2021-12-08T18:39:08Z
#106
🌟 Selected for report: ye0lde
Also found by: 0x0x0x, GiveMeTestEther, WatchPug
10.3016 USDC - $10.30
WatchPug
function outstandingArbTokens() public view returns (uint256 outstanding) { outstanding = 0;
Setting uint256 variables to 0 is redundant as they default to 0.
#0 - 0xScotch
2021-12-09T22:28:49Z
#80
#1 - GalloDaSballo
2021-12-27T22:35:41Z
Duplicate of #80
🌟 Selected for report: robee
Also found by: GiveMeTestEther, WatchPug, hyh, ye0lde
WatchPug
For the storage variables that will be accessed multiple times, cache them in the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
globalAdmin in Permissions#reassignGlobalAdmin()
function reassignGlobalAdmin(address _admin) external onlyRole(TIMELOCK_ROLE, "Only timelock can assign roles") { _swapRole(_admin, globalAdmin, TIMELOCK_ROLE); _swapRole(_admin, globalAdmin, ADMIN_ROLE); _swapRole(_admin, globalAdmin, GOVERNOR_ROLE); _swapRole(_admin, globalAdmin, MONETARY_BURNER_ROLE); _swapRole(_admin, globalAdmin, MONETARY_MINTER_ROLE); _swapRole(_admin, globalAdmin, STABILIZER_NODE_ROLE); _swapRole(_admin, globalAdmin, LIQUIDITY_MINE_ROLE); _swapRole(_admin, globalAdmin, AUCTION_ROLE); _swapRole(_admin, globalAdmin, REWARD_THROTTLE_ROLE); globalAdmin = _admin; }
verifiers[from] in TransferService#verifyTransfer()
verifiers[to] in TransferService#verifyTransfer()
focalLength in RewardDistributor.sol#_resetFocalPoint()
function _resetFocalPoint(uint256 id, uint256 endTime) internal { uint8 index = _getFocalIndex(id); FocalPoint storage newFocal = focalPoints[index]; newFocal.id = id; newFocal.focalLength = focalLength; newFocal.endTime = endTime; newFocal.rewarded = 0; newFocal.vested = 0; newFocal.lastVestingTime = endTime - focalLength; }
#0 - 0xScotch
2021-12-09T22:57:09Z
#161
#1 - GalloDaSballo
2022-01-16T14:58:21Z
Duplicate of #161
7.4171 USDC - $7.42
WatchPug
Checking uint256 variables >= 0 is redundant as they always >= 0.
Instances include:
function setLpProfitCut(uint256 _profitCut) public onlyRole(ADMIN_ROLE, "Must have admin privs") { require(_profitCut >= 0 && _profitCut <= 1000, "Must be between 0 and 100%"); lpProfitCut = _profitCut; }
_profitCut >= 0 at L170.
function setDelay(uint256 _delay) public onlyRole(GOVERNOR_ROLE, "Must have timelock role") { require( _delay >= 0 && _delay < gracePeriod, "Timelock::setDelay: Delay must not be greater equal to zero and less than gracePeriod" ); delay = _delay; emit NewDelay(delay); }
_delay >= 0 at L71.
#0 - GalloDaSballo
2021-12-30T16:13:57Z
Agree with finding, by definition it's either 0 or positive
25.436 USDC - $25.44
WatchPug
function unbondAndBreak(uint256 amount) external { require(amount > 0, "Cannot unbond 0"); uint256 bondedBalance = balanceOfBonded(msg.sender); require(bondedBalance > 0, "< bonded balance"); require(amount <= bondedBalance, "< bonded balance"); // Avoid leaving dust behind if (amount.add(1e16) > bondedBalance) { amount = bondedBalance; } miningService.onUnbond(msg.sender, amount); _unbondAndBreak(amount); }
L121, the check of bondedBalance > 0 is unnecessary, since the L122 already included the same check.
function transferAndCall(address to, uint value, bytes calldata data) external returns (bool) { require(to != address(0) || to != address(this)); uint256 balance = balanceOf(msg.sender); require(balance >= value, "ERC20Permit: transfer amount exceeds balance"); _transfer(msg.sender, to, value); return ITransferReceiver(to).onTokenTransfer(msg.sender, value, data); }
L121-L122, the check of balance >= value is unnecessary, since the L124 already included the same check.
#0 - GalloDaSballo
2022-01-01T15:01:26Z
Agree with the finding, the additional check is implicit and as such redundant
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
if (profit > 0) { uint256 lpCut = profit.mul(lpProfitCut).div(1000); collateralToken.safeTransfer(address(rewardThrottle), lpCut); rewardThrottle.handleReward(); }
Given that lpProfitCut can be 0, checking if lpProfitCut > 0 can avoid unnecessary code execution (including external calls) and save some gas.
#0 - GalloDaSballo
2022-01-07T00:44:56Z
Agree with finding, however, notice that lpProfitCut is a storage variable, so if you're going to read it more than once you'll also want to cache it's value.
It may be be ideal to rewrite as
uint256 cut = lpProfitCut if (profit > 0 && cut > 0) { uint256 lpCut = profit.mul(cut).div(1000);
Also noticed that you're using 1k instead of the more usual 10_000 (BPS math) Lastly you may want to store the max value in a constant (probably some other warden also flagged it), to avoid magic values
10.3016 USDC - $10.30
WatchPug
Using ++i is more gas efficient than i++, especially in a loop.
For example:
for (uint i = 0; i < _period; i++) { pegObservations.push(0); }
for (uint i = auctionAverageLookback; i < _lookback; i++) { pegObservations.push(0); }
for (uint i = 0; i < sampleMemory; i++) { samples.push(); }
for (uint256 i = 0; i < sampleMemory; i++ ) { tempCount += 1; liveSample = samples[_getIndexOfSample(tempCount)]; liveSample.timestamp = currentTimestamp; liveSample.cumulativeValue = currentCumulative; currentCumulative += addition; currentTimestamp += uint64(sampleLength); }
for (uint256 i = 0; i < sampleMemory; i++ ) { tempCount += 1; liveSample = samples[_getIndexOfSample(tempCount)]; liveSample.timestamp = currentTimestamp; liveSample.cumulativeValue = currentCumulative; currentCumulative += addition; currentTimestamp += uint64(sampleLength); }
for (uint i = sampleMemory; i < _sampleMemory; i++) { samples.push(); }
for (uint i; i < path.length - 1; i++) { (uint reserveIn, uint reserveOut) = getReserves(factory, path[i], path[i + 1]); amounts[i + 1] = getAmountOut(amounts[i], reserveIn, reserveOut); }
#0 - GalloDaSballo
2021-12-31T19:55:11Z
Pre-increment is cheaper because no intermediary value is stored, finding is valid
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
function _getFirstSample() private view returns (Sample storage firstSample) { uint32 sampleIndex = _getIndexOfSample(counter); // no overflow issue. if sampleIndex + 1 overflows, result is still zero. uint32 firstSampleIndex = uint32((sampleIndex + 1) % sampleMemory); firstSample = samples[firstSampleIndex]; }
Change to:
function _getFirstSample() private view returns (Sample storage firstSample) { // no overflow issue. if sampleIndex + 1 overflows, result is still zero. firstSample = samples[(counter + 1) % sampleMemory]; }
#0 - 0xScotch
2021-12-06T19:20:48Z
#252 points out an issue with the logic and will be fixed accordingly.
#1 - GalloDaSballo
2022-01-07T00:48:59Z
In principle I agree with the finding and believe it would save gas. Not super sure if this addresses the other concerns other wardens raised
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
function getRealBurnBudget( uint256 maxBurnSpend, uint256 premiumExcess ) public view returns(uint256) { // Returning maxBurnSpend = maximum supply burn with no reserve ratio improvement // Returning premiumExcess = maximum reserve ratio improvement with no real supply burn if (premiumExcess > maxBurnSpend) { return premiumExcess; } uint256 usableExcess = maxBurnSpend.sub(premiumExcess); if (usableExcess == 0) { return premiumExcess; } uint256 burnable = consult(usableExcess); return premiumExcess + burnable; }
L82-84 if (maxBurnSpend == premiumExcess) can be combined with L76-78.
Change to:
function getRealBurnBudget( uint256 maxBurnSpend, uint256 premiumExcess ) public view returns(uint256) { // Returning maxBurnSpend = maximum supply burn with no reserve ratio improvement // Returning premiumExcess = maximum reserve ratio improvement with no real supply burn if (premiumExcess >= maxBurnSpend) { return premiumExcess; } uint256 usableExcess = maxBurnSpend.sub(premiumExcess); uint256 burnable = consult(usableExcess); return premiumExcess + burnable; }
#0 - GalloDaSballo
2022-01-16T14:15:43Z
Pretty clever optimization
#1 - GalloDaSballo
2022-01-16T14:17:10Z
Tecnically just avoids an if which should be a JUMPI plus the comparison
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
function outstandingArbTokens() public view returns (uint256 outstanding) { outstanding = 0; for (uint256 i = replenishingIndex; i < auctionIds.length; i = i + 1) { uint256 claimable = auction.balanceOfArbTokens( auctionIds[i], address(this) ); outstanding = outstanding + claimable; } return outstanding; }
At L116, returning outstanding is redundant as named returns will be automatically handled.
#0 - 0xScotch
2021-12-06T19:24:08Z
Seems more readable to explicitly return them.
#1 - GalloDaSballo
2022-01-16T14:19:07Z
Agree with the warden in that the return variable is already implicit in the function signature. Don't mind a nofix as I agree that readability is fine as of now.
Note for the Sponsor @0xScotch : outstanding is assigned a 0, but because you declared it in the function signature, it's already set to 0
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.
For example:
function epochAPR(uint256 epoch) public view returns (uint256) { // This returns an implied APR based on the distributed rewards and bonded LP at the given epoch State memory epochState = _state[epoch]; uint256 bondedValue = epochState.bondedValue; if (bondedValue == 0) { bondedValue = bonding.averageBondedValue(epoch); if (bondedValue == 0) { return 0; } } // 10000 = 100% return epochState.rewarded.mul(10000).mul(dao.epochsPerYear()).div(bondedValue); }
dao.epochsPerYear() can be cached, Since epochAPR() is used in for loops:
function averageAPR(uint256 startEpoch, uint256 endEpoch) public view returns (uint256) { require(startEpoch < endEpoch, "Start cannot be before the end"); uint256 totalAPR = 0; for (uint256 i = startEpoch; i < endEpoch; i += 1) { totalAPR = totalAPR.add(epochAPR(i)); } return totalAPR / (endEpoch - startEpoch); }
Consider changing to function epochAPR(uint256 epoch, uint256 epochsPerYear) and cache the result of dao.epochsPerYear() before the for loop.
#0 - 0xScotch
2021-12-09T22:31:15Z
#274
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
function getPegDeltaFrequency() public view returns (uint256) { uint256 initialIndex = 0; uint256 index; if (count > auctionAverageLookback) { initialIndex = count - auctionAverageLookback; } uint256 total = 0; for (uint256 i = initialIndex; i < count; ++i) { index = _getIndexOfObservation(i); total = total + pegObservations[index]; } return total * 10000 / auctionAverageLookback; }
Change to:
function getPegDeltaFrequency() public view returns (uint256) { uint256 availablePegObservationsCount; { uint256 auctionAverageLookback_ = auctionAverageLookback; uint256 count_ = count; availablePegObservationsCount = count_ > auctionAverageLookback_ ? auctionAverageLookback_ : count_; } uint256 total = 0; for (uint256 i = 0; i < availablePegObservationsCount; ++i) { total += pegObservations[i]; } return total * 10000 / availablePegObservationsCount; }
#0 - GalloDaSballo
2022-01-16T14:27:33Z
I think the finding can help, however I have a note on it:
Code from warden <img width="876" alt="Screenshot 2022-01-16 at 15 24 05" src="https://user-images.githubusercontent.com/13383782/149663951-7f7b85bd-a876-4912-a908-779f561c48a5.png">
<img width="786" alt="Screenshot 2022-01-16 at 15 24 22" src="https://user-images.githubusercontent.com/13383782/149663961-9be3b32f-9af9-44c6-847f-1ea34b9ccf1b.png"> The warden is using supporting memory variables but they are used only once, the extra 3 + 3 gas for storage and read are wasted🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
function _handleStakePadding(address account, uint256 amount) internal { uint256 totalRewardedWithStakePadding = totalDeclaredReward().add(totalStakePadding()); uint256 INITIAL_STAKE_SHARE_MULTIPLE = 1e6; uint256 bondedTotal = totalBonded(); uint256 newStakePadding = bondedTotal == 0 ? totalDeclaredReward() == 0 ? amount.mul(INITIAL_STAKE_SHARE_MULTIPLE) : 0 : totalRewardedWithStakePadding.mul(amount).div(bondedTotal); _addToStakePadding(account, newStakePadding); }
function totalStakePadding() public view returns(uint256) { return _globalStakePadding; }
In AbstractRewardMine.sol#_handleStakePadding(), at L180, the internal function call to totalStakePadding() is unnecessary, which can be replaced by direct storage read to save gas.
The same issue exists in _getFullyPaddedReward().
#0 - GalloDaSballo
2022-01-16T14:29:50Z
Finding is valid and would save a JUMP, personally don't mind a nofix to maintain readability (or you could switch to internal functions which the optimizer should inline)
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
Unused storage variables in contracts use up storage slots and increase contract size and gas usage at deployment and initialization.
Instances include:
address public uniswapV2Factory;
#0 - GalloDaSballo
2022-01-16T14:30:44Z
Variable is unused, and it's in storage increasing depoyment and initialize cost
🌟 Selected for report: WatchPug
WatchPug
ERC20 public rewardToken; IBurnMintableERC20 public malt;
rewardToken and malt will never change, use immutable variable instead of storage variable can save gas.
Other examples include:
address public swingTrader;
ERC20 public malt; ERC20 public rewardToken; ERC20 public lpToken; IUniswapV2Router02 public router; address public uniswapV2Factory;
ERC20 public collateralToken; ERC20 public malt; IDexHandler public dexHandler; IRewardThrottle public rewardThrottle;
#0 - 0xScotch
2021-12-08T18:42:06Z
#105
#1 - GalloDaSballo
2022-01-16T14:53:55Z
Finding is valid and not a duplicate of #105
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
uint64 blockTimestamp = uint64(block.timestamp % 2**64);
Use uint64(n) can cut off higher-order bits already, n % 2**64 is redundant.
See: https://docs.soliditylang.org/en/v0.8.10/types.html#explicit-conversions
Change to:
uint64 blockTimestamp = uint64(block.timestamp);
#0 - GalloDaSballo
2022-01-16T14:54:41Z
Agree that uint64 will cut out higher bits, as such the modulo is redundant
15.2616 USDC - $15.26
WatchPug
For the arithmetic operations that will never over/underflow, using SafeMath will cost more gas.
For example:
if (amountTokens > unclaimedArbTokens) { unclaimedArbTokens = 0; } else { unclaimedArbTokens = unclaimedArbTokens.sub(amountTokens); }
unclaimedArbTokens - amountTokens will never underflow.
Change to:
if (amountTokens >= unclaimedArbTokens) { unclaimedArbTokens = 0; } else { unclaimedArbTokens -= amountTokens; }
if (premiumExcess > maxBurnSpend) { return premiumExcess; } uint256 usableExcess = maxBurnSpend.sub(premiumExcess);
maxBurnSpend - premiumExcess will never underflow.
Change to:
if (premiumExcess > maxBurnSpend) { return premiumExcess; } uint256 usableExcess = maxBurnSpend - premiumExcess;
#0 - GalloDaSballo
2021-12-31T19:12:13Z
Agree with the finding, typically safeMath adds an extra 30 / 40 gas per check, that said, it's ok to keep it to make the codebase more consistent
🌟 Selected for report: WatchPug
56.5245 USDC - $56.52
WatchPug
function _getCurrentSample() private view returns (Sample storage currentSample) { // Active sample is always counter - 1. Counter is the in progress sample uint32 currentSampleIndex = _getIndexOfSample(counter - 1); currentSample = samples[currentSampleIndex]; }
The local variable currentSampleIndex is used only once. Making the expression inline can save gas.
Similar issue exists in _getFirstSample(), _getNthSample(), AuctionBurnReserveSkew.sol#getRealBurnBudget(), MovingAverage.sol#_getFirstSample().
Change to:
function _getCurrentSample() private view returns (Sample storage currentSample) { // Active sample is always counter - 1. Counter is the in progress sample currentSample = samples[_getIndexOfSample(counter - 1)]; }
#0 - GalloDaSballo
2022-01-16T14:57:40Z
Agree that this would save gas, specifically 3 for storing in memory and another 3 for reading from it