Platform: Code4rena
Start Date: 09/09/2021
Pot Size: $100,000 SUSHI
Total HM: 4
Participants: 11
Period: 7 days
Judge: ghoulsol
Total Solo HM: 3
Id: 28
League: ETH
Rank: 7/11
Findings: 2
Award: $4,420.82
🌟 Selected for report: 5
🚀 Solo Findings: 0
67.3096 SUSHI - $675.12
leastwood
TimelockController.sol acts as an auxiliary contract to the MISO platform's core contracts. Therefore, this issue is not of high risk as not all users wanting to auction tokens will use this contract for governance behaviour. The TimelockController.sol enables a governance framework to enforce a timelock on any proposals, giving users time to exit before a potentially dangerous maintenance operation is applied. However, the executeBatch() is vulnerable to reentrancy, enabling privilege escalation for any account with the EXECUTOR role to ADMIN.
Bug outlined here. Fix is outlined in this commit.
Sourced from publicly disclosed post by Immnuefi.
Update Openzeppelin library to a version containing the commit fixing the bug (mentioned above). Tag v3.4.2-solc-0.7 in Openzeppelin's Github repository is an example of a compatible library that contains the aforementioned bug fix.
#0 - Clearwood
2021-09-16T05:05:57Z
As the TimeLock Controller is currently used nowhere in the project and its a known issue, I would propose to put down the severity of this issue.
#1 - ghoul-sol
2021-10-05T17:41:34Z
I double checked deployment scripts and indeed, Timelock is not used anywhere in the setup and for that reason I think low risk is justified because the only risk is the devs forgetting to upgrade the contract in the future. Also, this is not part of core protocol.
#2 - Clearwood
2021-10-05T18:33:32Z
🌟 Selected for report: leastwood
149.5769 SUSHI - $1,500.26
leastwood
If the current template ID is removed from MISOLauncher.sol, the function removeLiquidityLauncherTemplate() does not accurately reflect this by deleting currentTemplateId[_templateId]. This may lead to users actively using a removed template, expecting the deployLauncher() function to succeed when it will revert instead.
https://github.com/sushiswap/miso/blob/master/contracts/MISOLauncher.sol#L323-L334
Manual code review
Consider removing currentTemplateId[_templateId] if the template to be removed by removeLiquidityLauncherTemplate() is the same template.
#0 - Clearwood
2021-09-23T03:39:46Z
good catch but risk is non existant
#1 - Clearwood
2021-10-05T18:32:23Z
#2 - ghoul-sol
2021-10-06T00:12:02Z
I guess removing is handled by trusted actor that will know to update it in multiple places however I think low risk is fair here.
40.3858 SUSHI - $405.07
leastwood
There are no assertions that _startPrice and _minimumPrice are significantly greater than the time difference used to calculate DutchAuction.priceDrop(). As a result, it is possible for values to be heavily truncated, leading to an incorrect priceDiff and resulting _currentPrice().
https://github.com/sushiswap/miso/blob/master/contracts/Auctions/DutchAuction.sol#L325-L332 https://github.com/sushiswap/miso/blob/master/contracts/Auctions/DutchAuction.sol#L415-L418
Manual code review
Consider enforcing a minimum amount difference between _startPrice and _minimumPrice in DutchAuction.initAuction().
#0 - Clearwood
2021-09-23T03:37:36Z
@leastwood If the values are reasonable, does it lead to any exploitable behavior? or does it just turn the DutchAuction into a crowdsale
#1 - Clearwood
2021-09-23T03:47:14Z
Duplicate of #108
6.2284 SUSHI - $62.47
leastwood
DutchAuction.clearingPrice() and HyperbolicAuction.clearingPrice() can both be improved such that the initial results for tokenPrice() and priceFunction() are cached in memory and used to return the correct output.
https://github.com/sushiswap/miso/blob/master/contracts/Auctions/DutchAuction.sol#L227-L234 https://github.com/sushiswap/miso/blob/master/contracts/Auctions/HyperbolicAuction.sol#L218-L224
Manual code review
Consider caching the results to limit the number SSTORE accesses.
#0 - Clearwood
2021-09-23T04:08:35Z
Duplicate of #105
🌟 Selected for report: leastwood
13.8408 SUSHI - $138.82
leastwood
If state variables are set in a contract's constructor and only read by other functions, the immutable keyword can be used to generate small gas savings upon deployment.
The following instances can be improved by such a fix:
PairsHelper.constructor() variables uniqueAddressesHelperAddress and wethAddress.MISOHelper.constructor() variable accessControls.CalculationsSushiswap.constructor() all variables.PostAuctionLauncher.constructor() variable weth.https://github.com/sushiswap/miso/blob/master/contracts/Liquidity/PostAuctionLauncher.sol#L105-L107 https://github.com/sushiswap/miso/blob/master/contracts/Helper/CalculationsSushiswap.sol#L33-L60 https://github.com/sushiswap/miso/blob/master/contracts/Helper/MISOHelper.sol#L723-L736 https://github.com/sushiswap/miso/blob/master/contracts/Helper/PairsHelper.sol#L39-L45
Manual code review
Consider implementing the above changes.
#0 - Clearwood
2021-09-23T04:09:14Z
For everything but the PostAuctionLauncher this is not relevant, as those are all helper functions
🌟 Selected for report: leastwood
13.8408 SUSHI - $138.82
leastwood
State variables can be consolidated in solidity to minimise the number of slots used by a contract upon deployment and in each struct instance. struct PLInfo in MISOHelper.sol can have all time values updated to uint64 datatypes. bool values in MISOLauncher.sol can be combined into a single slot.
https://github.com/sushiswap/miso/blob/master/contracts/Helper/MISOHelper.sol#L510-L522 https://github.com/sushiswap/miso/blob/master/contracts/MISOLauncher.sol#L61-L102
Manual code review
Consider consolidating the above.
#0 - Clearwood
2021-09-23T04:06:51Z
As MisoHelper is only used by UI consolidation there is not really needed
🌟 Selected for report: leastwood
149.5769 SUSHI - $1,500.26
leastwood
There are a few instances whereby a malicious actor could monitor the blockchain for instances of bytecode matching any of MISO's suite of smart contracts. These smart contracts can be logically separated into two types, core and auxiliary contracts. MISO's core contracts are deployed using an automated script and MISO's auxiliary contracts are deployed by users to suit their auction requirements. There is currently no mechanism in place to ensure that a contract is deployed and initialized safely. By using a two-step process, MISO is allowing attackers to frontrun their deployments and potentially DOS further upgrades. While MISO's base contracts have already been deployed, any further upgrades to mitigate vulnerabilities will be prone to frontrunning. Additionally, auxiliary contracts deployed by users are also vulnerable, i.e. MISOFermenter.sol and GnosisSafeFactory. DOS attacks can be sustained by malicious actors as deployment costs far outweigh the cost to frontrun and initialize these contracts.
https://github.com/sushiswap/miso/blob/master/deploy/main.ts https://github.com/sushiswap/miso/blob/master/contracts/MISOFermenter.sol#L98-L105 https://github.com/sushiswap/miso/blob/master/contracts/Vault/GnosisSafeFactory.sol#L44-L51
Manual code review.
Contracts implementing access control can be deployed such that their constructor() assigns the admin role to the deployer account. Upon initialization, this role can be removed from the deployer account and assigned to the correct admin account. This guarantees frontrunning prevention as long as the deployer's private key is not compromised before or during deployment.
#0 - Clearwood
2021-09-16T04:45:56Z
We are consiouscly using initializers as a pattern. With contracts that are actively used already like the auctions, initializers are called by factories after a clone has been created.
#1 - ghoul-sol
2021-10-05T18:05:26Z
per sponsor comment i think this is a low risk