Platform: Code4rena
Start Date: 27/01/2022
Pot Size: $90,000 USDC
Total HM: 21
Participants: 33
Period: 7 days
Judge: Jack the Pug
Total Solo HM: 14
Id: 78
League: ETH
Rank: 9/33
Findings: 5
Award: $2,161.83
🌟 Selected for report: 3
🚀 Solo Findings: 0
🌟 Selected for report: camden
Also found by: GeekyLumberjack, kirk-baird, shw
362.2874 USDC - $362.29
camden
https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/UniswapHelper.sol#L138
In the Uniswap helper, generateFLNQuote is public, so any user can generate the latest quote. If you call this twice in any block, then the two latest flan quotes will have a blockProduced value of the current block's number.
These quotes are used in the _ensurePriceStability function. The last require statement here is key:
https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/UniswapHelper.sol#L283-L285
This function will revert if this statement is false:
localFlanQuotes[0].blockProduced - localFlanQuotes[1].blockProduced > VARS.minQuoteWaitDuration
Since VARS.minQuoteWaitDuration is a uint256, it is at least 0
localFlanQuotes[0].blockProduced - localFlanQuotes[1].blockProduced > 0
But, as we've shown above, we can create a transaction in every block that will make localFlanQuotes[0].blockProduced - localFlanQuotes[1].blockProduced == 0. In any block we can make any call to _ensurePriceStability revert.
_ensurePriceStability is called in the ensurePriceStability modifier:
https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/UniswapHelper.sol#L70
This modifier is used in stabilizeFlan:
https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/UniswapHelper.sol#L162
Lastly, stabilizeFlan is used in migrate in Limbo.sol
https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/Limbo.sol#L234
Therefore, we can grief a migration in any block. In reality, the minQuoteWaitDuration would be set to a much higher value than 0, making this even easier to grief for people (you only need to call generateFLNQuote every minQuoteWaitDuration - 1 blocks to be safe).
Mitigation is to just use a time weighted oracle for uniswap.
#0 - gititGoro
2022-02-02T01:59:21Z
I appreciate the write up. You're not technically incorrect on the problem. The solution isn't idea because Uniswap can't tell us what's happening on Behodler.
UniswapHelper can be replaced without much trouble. So if the oracle functionality does fail, we can deploy a better one. But for now, I doubt that the incentive exists to perpetually grief migrations on Limbo. If someone does start to grief, we can add flash governance to the flan quote generation and then burn the EYE belonging to griefers but I was reluctant to call on the big guns right from the start.
#1 - gititGoro
2022-02-03T19:51:27Z
I've changed this to confirmed because a cryptoeconomic layer should be added. Flash governance for sampling an oracle is too extreme and adding a require to force the duration can still be griefed.
Instead I think the solution is to force the caller to pay EYE if the interval is below the min required. The EYE is then burnt. So the idea is that if you wish to grief migrations, it's going to cost you more than just gas and the community will benefit from your griefing.
#2 - gititGoro
2022-05-29T00:09:07Z
894.5368 USDC - $894.54
camden
The attack here allows the attacker to prevent migrations.
The attack here is recoverable because we can just call buyFlanAndBurn (f it worked as expected) with SCX as the input token to buy Flan with the extra SCX, then run the migration again.
The attack here is simple:
My proof of concept test. You should be able to use this directly in the thig https://gist.github.com/CamdenClark/b6841ac7a63e868d90eff7d9a40e3e0a
localSCXBalance is the SCX balance of the uniswap helper. https://github.com/code-423n4/2022-01-behodler/blob/cedb81273f6daf2ee39ec765eef5ba74f21b2c6e/contracts/UniswapHelper.sol#L163
But, the caller of stablizeFlan assumes that the rectangleOfFairness parameter is going to be equal to the amount of SCX that was sent
https://github.com/code-423n4/2022-01-behodler/blob/cedb81273f6daf2ee39ec765eef5ba74f21b2c6e/contracts/Limbo.sol#L234
The mitigation could be to do >= instead of == so sending tokens can't grief this.
Beyond this though, why do you need to pass in rectangleOfFairness if we're requiring it to be a function of the localSCXBalance anyways? https://github.com/code-423n4/2022-01-behodler/blob/cedb81273f6daf2ee39ec765eef5ba74f21b2c6e/contracts/UniswapHelper.sol#L167
#0 - gititGoro
2022-02-02T02:33:25Z
I really like the way you've thought about this and I feel a little sad about not confirming it but it overlaps enough with a prior report that the solution is duplicated.
It's interesting to think about this issue because it's the type of tight rope walk between incentives and code enforcement. In this scenario, the net results of the griefing will be both a higher flan and scx price. So in exchange for a timely migration, we get a boost to flan and scx (which is precisely the goal of a migration from Limbo's perspective). Eventually we get the migration we wanted but only after some price assist from a griefer. For this cryptoeconomic reason, I've marked it as acknowledged rather than confirmed.
Still I appreciate the depth with which you've thought about this and I hope the Behodler community sees more of you after this audit.
#1 - jack-the-pug
2022-02-27T08:32:32Z
Good catch! Thank you for creating the proof of concept test script. The Code4rena community needs more wardens like you!
🌟 Selected for report: camden
Also found by: kirk-baird
894.5368 USDC - $894.54
camden
The impact here is that a user can, right at the end of the voting period, flip the decision without triggering the logic to extend the vote duration. The user doesn't even have to be very sophisticated: they can just send one vote in one transaction to go to 0, then in a subsequent transaction send enough to flip the vote.
https://github.com/code-423n4/2022-01-behodler/blob/608cec2e297867e4d954a63fecd720e80c1d5ae8/contracts/DAO/LimboDAO.sol#L281 You can send exactly enough fate to send the fate amount to 0, then send fate to change the vote. You'll never trigger this logic.
On the first call, to send the currentProposalState.fate to 0, (fate + currentFate) * fate == 0, so we won't extend the proposal state.
Then, on the second call, to actually change the vote, fate * currentFate == 0 because currentFate is 0.
Make sure that going to 0 is equivalent to a flip, but going away from 0 isn't a flip.
#0 - gititGoro
2022-02-02T03:00:27Z
Changing the logic to include this edge case can get a little convoluted. One thing I thought of is to change the condition to currentFatefate<0 && currrentFatecurrentFate>fate*fate but then moving from 0 o positive won't flip the vote. What about requiring the square of your vote to not equal the currentFate and reverting if not? In other words, your vote needs to either have no flipping impact or clearly be intended to flip, not just to cancel out all other votes.
#1 - gititGoro
2022-02-12T02:38:30Z
After some consideration, I'm going to implement the square of votes != currentVote rule as a tie makes no sense in the context of whether to execute.
🌟 Selected for report: Ruhum
Also found by: 0v3rf10w, CertoraInc, Dravee, camden, gzeon, hyh, sirhashalot
10.4613 USDC - $10.46
camden
These are view functions that don't change any state in the PyroToken, and the redeemRate property isn't used for anything.
https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/FlanBackstop.sol#L93-L94 https://github.com/code-423n4/2022-01-behodler/blob/71d8e0cfd9388f975d6a90dffba9b502b222bdfe/contracts/FlanBackstop.sol#L111-L113
#0 - jack-the-pug
2022-02-27T13:11:54Z
Dup #79