Platform: Code4rena
Start Date: 07/01/2022
Pot Size: $80,000 USDC
Total HM: 21
Participants: 37
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 71
League: ETH
Rank: 4/37
Findings: 4
Award: $4,614.61
🌟 Selected for report: 4
🚀 Solo Findings: 2
🌟 Selected for report: sirhashalot
3799.5139 INSURE - $1,329.83
2306.8477 USDC - $2,306.85
sirhashalot
The Vault.sol contract has two address state variables, the keeper variable and the controller variable, which are both permitted to be the zero address. If both variables are zero simultaneously, any address can burn the available funds (available funds = balance - totalDebt) by sending these tokens to the zero address with the unprotected utilitize() function. If a user has no totalDebt, the user can lose their entire underlying token balance because of this.
The problematic utilize() function is found here. To see how the two preconditions can occur:
setKeeper() function found here. If this function is not called, the keeper variable will retain the default value of address(0), which bypasses the only access control for the utilize function.If both address variables are left at their defaults of address(0), then the safeTransfer() call on line 348 would send the tokens to address(0).
Add the following line to the very beginning of the utilize() function:
require(address(controller) != address(0))
This check is already found in many other functions in Vault.sol, including the _unutilize() function.
#0 - oishun1112
2022-01-31T04:53:37Z
🌟 Selected for report: sirhashalot
379.9514 INSURE - $132.98
230.6848 USDC - $230.68
sirhashalot
The InsureDao protocol is a type of DEX and therefore should protect users against frontrunning and sandwich attacks. The approach used by Uniswap is to set bounds on the slippage that can occur in a trade by specifying a minimum amount for the DEX user to receive, as seen in the Uniswap addLiquity function. In InsureDao, there is no slippage protection, so an attacker could use a flashloan to cause a large change in liquidity of a trading pair, causing a change in price for the pair. Since a user cannot set a minimum value they expect to receive, they could receive far fewer assets than expected, and the attacker may profit.
Here is a similar finding in a public audit and here is an example of this weakness in a DEX.
The deposit() function in CDSTemplate.sol should have some guarantees for the user to receive a minimum amount of value. No guarantees or minimum values are currently used. With the current code, is an attacker makes a large change to the totalSupply() or totalLiquidity() values of the pool, they will alter the _mintAmount calculation. Because this can occur in the same block as the user's deposit, the user will be unaware of what happened until it is too late.
Add slippage protection to provide guarantees for the user that they will receive at least a certain amount of tokens in return for their deposit.
#0 - oishun1112
2022-01-18T04:39:09Z
Premium income reduce the return amount of LP token. So, to attack(?), attacker has to buy huge amount of insurance that is no incentive on the attacker.
You referred about sandwich attack. I guess you are actually talking about insure() function. Sandwich attack won't be happen unless bought insurance(more like NFT) can be sold back in the market.
#1 - 0xean
2022-01-27T22:17:25Z
Agreed due to the async nature of transactions being mined this would be a beneficial feature to a caller even if not beneficial for the attacker.
Downgrading severity.
🌟 Selected for report: sirhashalot
379.9514 INSURE - $132.98
230.6848 USDC - $230.68
sirhashalot
The comment for the return value of the getCDS() function in Registry.sol is incorrectly copied from elsewhere, possibly the confirmExistence() function. The return value is an address, not a boolean. This is considered low risk based on C4's risk ratings.
The problematic comment is from the getCDS() function here. It is an incorrect duplicate of the comment for the confirmExistence() function found here.
Replace the comment with something like @return CDS contract address, which is used to describe this value in the setCDS() function.
#0 - 0xean
2022-01-27T23:04:38Z
1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.
🌟 Selected for report: defsec
Also found by: sirhashalot
170.9781 INSURE - $59.84
103.8081 USDC - $103.81
sirhashalot
Before the transfer() function is called, the token owner must approve a transfer of this amount (or greater). However, the transferFrom() function in InsureDAOERC20.sol calls these functions in the reverse order. This could lead to a denial of service scenario in situations where the transferFrom() function is required by other contracts. For example, the Vault.sol contract calls safeTransferFrom() and in turn calls transferFrom(), which would require users to manually call the token's underlying approve function to fix the error in InsureDAOERC20.sol.
The issue is in the InsureDAOERC20.sol transferFrom function
Compare the order of the _approve() and _transfer() calls to the OpenZeppelin ERC20 implemention.
Move the _transfer() function call to immediately after the _approve() call to mimic the OpenZeppelin ERC20 implemention.
#0 - oishun1112
2022-01-18T08:15:23Z
🌟 Selected for report: sirhashalot
62.2711 INSURE - $21.79
32.6923 USDC - $32.69
sirhashalot
The Vault.sol contract contains several state variables of type address. There is no need to cast these variable to type address because they are already of type address. Removing the cast function can save gas.
The token address state variable is unnecessarily cast to address type in two places in Vault.sol:
Remove the unnecessary address cast from address variables.
🌟 Selected for report: robee
Also found by: sirhashalot
28.022 INSURE - $9.81
14.7115 USDC - $14.71
sirhashalot
The state variable pendingEnd is declared but not used in IndexTemplate.sol. Removing the declaration of these unused variables would save gas.
The unused state variable is pendingEnd in IndexTemplate.sol
Remove unused and unnecessary state variables
#0 - oishun1112
2022-01-13T11:09:11Z
6.1284 INSURE - $2.14
3.2174 USDC - $3.22
sirhashalot
In Solidity version starting with 0.8.0, all math operations check for over/underflows. This adds overhead, and in situations where an over/underflow cannot happen, it makes sense to remove the overhead to save gas using "unchecked". This is further described in the official Solidity docs.
Referencing the OpenZeppelin ERC20 implemention, we can see four places where "uncheck" can be added:
Use "unchecked" where there is no overflow risk to save gas.
#0 - 0xean
2022-01-28T02:49:34Z
dupe of #66
🌟 Selected for report: 0xngndev
Also found by: Dravee, Jujic, Meta0xNull, defsec, p4st13r4, robee, sirhashalot, tqts
2.9784 INSURE - $1.04
1.5637 USDC - $1.56
sirhashalot
Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas as documented publicly
There are multiple examples of this gas optimization opportunity, including but not limited to:
Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met. Alternatively, the code could be modified to use custom errors, introduced in Solidity 0.8.4: https://blog.soliditylang.org/2021/04/21/custom-errors/
#0 - oishun1112
2022-01-13T04:49:24Z
#1 - oishun1112
2022-01-13T04:49:33Z
#2 - 0xean
2022-01-27T23:58:00Z
dupe of #87