Platform: Code4rena
Start Date: 27/05/2021
Pot Size: $100,000 USDC
Total HM: 12
Participants: 7
Period: 7 days
Judge: LSDan
Total Solo HM: 10
Id: 12
League: ETH
Rank: 3/7
Findings: 4
Award: $12,603.62
🌟 Selected for report: 3
🚀 Solo Findings: 3
🌟 Selected for report: shw
1933.0174 USDC - $1,933.02
shw
A user can artificially increase the chi accrual (after maturity) by flash borrow on Compound, which affects the exchange rate used by the chi oracle. As a result, the user redeems more underlying tokens with the same amount of fyTokens since the accrual is larger than before.
The exchangeRateStored used by chi oracle is calculated based on the totalBorrows of CToken, which can be artificially increased by a large amount of borrow operated on Compound. Consider a user performing the following steps in a single transaction (assuming that the fyToken is matured):
redeemThe user only needs to pay for the gas fees of borrowing and repaying (since they happen in the same transaction) but can redeem more underlying tokens than a regular redeem.
Referenced code: CompoundMultiOracle.sol#L46 FYToken.sol#L125 FYToken.sol#L132-L143
Make the chi accrual time-weighted to mitigate the manipulation caused by flash borrow and repay.
#0 - alcueca
2021-06-04T08:58:49Z
If this is true, that means that we don't understand how exchangeRateStored works (quite likely).
Our understanding was that exchangeRateStored is an increasing accumulator, same as chi in MakerDAO which is both an exchange rate and an accummulator.
If exchangeRateStored can go down in value, as well as up, we might have to revisit how we source it.
#1 - berlinhardfork
2023-06-06T06:59:12Z
Isn't valid issue. totalBorrows is increased while totalCash is decreased.
So, the exchangeRateStored isn't going up or going down.
🌟 Selected for report: shw
4295.5942 USDC - $4,295.59
shw
It is possible for an attacker to intendedly create a fake Join corresponding to a specific token beforehand to make Wand unable to deploy the actual Join, causing a DoS attack.
The address of Join corresponding to an underlying asset is determined as follows and thus unique:
Join join = new Join{salt: keccak256(abi.encodePacked(asset))}();
Besides, the function createJoin in the contract JoinFactory is permissionless: Anyone can create the Join corresponding to the asset. An attacker could then deploy a large number of Joins with different common underlying assets (e.g., DAI, USDC, ETH) before the Wand deploying them. The attempt of deploying these Joins by Wand would fail since the attacker had occupied the desired addresses with fake Joins, resulting in a DoS attack.
Moreover, the attacker can also perform DoS attacks on newly added assets: He monitors the mempool to find transactions calling the function addAsset of Wand and front-runs them to create the corresponding Join to make the benign transaction fail.
Referenced code: JoinFactory.sol#L64-L75 Wand.sol#L53
Enable access control in createJoin (e.g., adding the auth modifier) and allow Wand to call it.
#0 - alcueca
2021-06-04T09:06:37Z
The issue exists, and we appreciate raising it.
The solution can't be adding auth to createJoin, since anyone can use CREATE2 to deploy a Join with their own factory, but with our Join bytecode, occupying the same address we would.
The correct mitigation, in our opinion, is to ditch CREATE2 and deploy Joins using CREATE instead.
As for the risk, such a DoS attack wouldn't cause a loss of funds, or an interruption on user service. It would cause a governance action to revert, which would be quickly fixed by deploying a new JoinFactory and replacing the Wand. Fortunately, no contract uses the Wand as a Join registry (maybe we should!).
I suggest the risk is downgraded to 1.
🌟 Selected for report: shw
4295.5942 USDC - $4,295.59
shw
According to the protocol design, users have to pay borrowing interest when repaying the debt with underlying tokens after maturity. However, a user can give his vault to Witch and then buy all his collateral using underlying tokens to avoid paying the interest. Besides, this bug could make users less incentivized to repay the debt before maturity and hold the underlying tokens until liquidation.
Ladle), which is his debt multiplied by the rate accrual (line 373).Witch by calling the function batch of Ladle with the operation GIVE.buy of Witch with the corresponding vaultId to buy all his collateral using underlying tokens.In the last step, the elapsed time (line 61) is equal to the current timestamp since the vault is never grabbed by Witch before, and thus the auction time of the vault, cauldron.auctions(vaultId), is 0 (the default mapping value). Therefore, the collateral is sold at a price of balances_.art/balances_.ink (line 74). The user can buy balances_.ink amount of collateral using balances_.art but not paying for borrowing fees.
Referenced code: Ladle.sol#L350 Ladle.sol#L368-L377 Ladle.sol#L267-L272 Cauldron.sol#L234-L252 Witch.sol#L61 Witch.sol#L74
Do not allow users to give vaults to Witch. To be more careful, require vaultOwners[vaultId] and cauldron.auctions(vaultId) to be non-zero at the beginning of function buy.
#0 - alcueca
2021-06-08T08:55:29Z
That's a good catch. The mitigation steps are right to avoid this being exploited by malicious users, but it would be better to fix the underlying issue.
The problem is that the Witch always applies a 1:1 exchange rate between underlying and fyToken, and that is not true after maturity. As long as this is not fixed, the protocol will lose money on after maturity liquidations.
#1 - alcueca
2021-06-08T09:08:38Z
More specifically, _debtInBase should be a Cauldron public function, and return (debtInFYToken, debtInBase). The Ladle would save a bit in deployment gas, the Witch would use it to find out the underlying / fyToken exchange rate.
#2 - alcueca
2021-06-08T09:28:35Z
However, since grab wouldn't be called on the Witch, the vault owner wouldn't be registered. With the liquidation being a Dutch auction, the vault owner would only get a portion of his collateral back after paying all the debt. The vault with the remaining collateral would be given to address(0).
There is a small protocol loss from miscalculated vault debt on vaults liquidated after maturity, but no user funds would be at risk.
I would propose a severity of 2, given there are monetary losses, however slight, to the protocol. The attack described can't happen, but it revealed a real issue.