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: 5/33
Findings: 5
Award: $6,637.35
🌟 Selected for report: 9
🚀 Solo Findings: 2
🌟 Selected for report: CertoraInc
1987.8596 USDC - $1,987.86
CertoraInc
updateCurrentProposal() modifier and makeProposal() function)The LimboDAO contract has a variable that indicates the current proposal - every time there can be only one proposal. The only way a proposal can be done and a new proposal can be registered is to finish the previous proposal by either accepting it and executing it or by rejecting it. If a proposal that can't succeed, like for example an UpdateMultipleSoulConfigProposal proposal that has too much tokens and not enough gas, will stuck the system if it will be accepted. Thats because its time will pass - the users won't be able to vote anymore (because the vote function will revert), and the proposal can't be executed - the execute function will revert. So the proposal won't be able to be done and the system will be stuck because new proposal won't be able to be registered.
When trying to call the executeCurrentProposal() function that activates the updateCurrentProposal() modifier, the modifier will check the balance of fate, it will see that it's positive and will call currentProposalState.proposal.orchestrateExecute() to execute the proposal. the proposal will revert and cancel it all (leaving the proposal as the current proposal with voting state).
When trying to call makeProposal() function to make a new proposal it will revert because the current proposal is not equal to address(0).
To sum up, the system can get to a "stuck" state if a bad proposal (proposal that can't be executed) is accepted.
#0 - gititGoro
2022-02-03T22:05:58Z
I'm so glad someone finally noticed this. So many issues logged skirted around this issue. A lot of issues were logged about adding too may token to the updateMultipleSoulProposal but the crux of the matter is that the proposal.execute() should be replaced with a call that returns a success boolean so that the DAO doesn't get stuck on broken proposals. Congratulations on spotting this.
#1 - jack-the-pug
2022-02-16T12:49:46Z
This is a good one, but I'm still going to downgrade this to medium as there is no fund at risk afaics.
#2 - gititGoro
2022-03-02T03:35:31Z
@jack-the-pug There is a funds risk. Limbo can be paused via flash governance. When paused, funds can't be withdrawn. The only way to unpause is with a proposal. If the DAO gets jammed up with a broken proposal contract then an attacker can pause Limbo and all staked funds will be locked permanently.
#3 - jack-the-pug
2022-03-02T04:08:14Z
@jack-the-pug There is a funds risk. Limbo can be paused via flash governance. When paused, funds can't be withdrawn. The only way to unpause is with a proposal. If the DAO gets jammed up with a broken proposal contract then an attacker can pause Limbo and all staked funds will be locked permanently.
Yeah, I agree that funds can be at risk indirectly, like the vector you described above, but only when the warden made a clear and persuasive presentation about how the funds can be at risk, then it can be a High.
Furthermore, this attack vector requires the community to misbehave or at least be imprudent, to pass a malicious proposal, which already lowers the severity of it.
#4 - gititGoro
2022-05-31T22:17:04Z
🌟 Selected for report: CertoraInc
1987.8596 USDC - $1,987.86
CertoraInc
safeTransfer() function)The flan contract must have balance (and must have more flan then we want to transfer) in order to allow flan transfers. If it doesn't have any balance, the safeTransfer, which is the only way to transfer flan, will call _transfer() function with amount = 0. It should check address(msg.sender)'s balance instead of address(this)'s balance.
function safeTransfer(address _to, uint256 _amount) external { uint256 flanBal = balanceOf(address(this)); // the problem is in this line uint256 flanToTransfer = _amount > flanBal ? flanBal : _amount; _transfer(_msgSender(), _to, flanToTransfer); }
#0 - jack-the-pug
2022-02-16T12:52:51Z
downgrade to medium as there is no fund at risk.
🌟 Selected for report: CertoraInc
Also found by: Randyyy
894.5368 USDC - $894.54
CertoraInc
stake() function)if a user has a pending reward and he call the stake function with amount = 0, he won't be able to get his reward (he won't get the reward, and the reward debt will cover the reward)
that's happening because the reward calculation is done only if the staked amount (given as a parameter) is greater than 0, and it updates the reward debt also if the amount is 0, so the reward debt will be updated without the user will be able to get his reward
#0 - gititGoro
2022-02-03T21:24:49Z
Good catch! I'd be interested in your mitigation step being provided.
To me, it looks like the simplest solution is just to remove that if statement. Users who stake zero will pay unnecessary gas costs but the contract shouldn't have to optimise gas consumption for undesired behaviour.
#1 - jack-the-pug
2022-02-27T08:11:25Z
Upgraded to Med as users can lose their rewards.
🌟 Selected for report: CertoraInc
662.6199 USDC - $662.62
CertoraInc
Similar to ERC20.approve, approveUnstake() is unsafe due to the fact that it set the allowance to a fixed number and doesn't increase or decrease it.
Usually, the ERC20.approve doesn't get much attention because they leave it to the user to make sure his operation is safe, however here the user cannot do it because the unstakeApproval state variable is private and there is no getter for it.
In ERC20.approve users can simply check the allowance and change it in the same transaction and eliminate the risk, but here it's impossible.
Users will not be able to change the allowance of the unstake without the risk of the frontrunning stealing like the classic ERC20.approve (there the risk can be removed).
This will cause users to not change allowance for users that they don't 100% trust which can be problematic
The function that sets the allowance to a fixed number: https://github.com/code-423n4/2022-01-behodler/blob/main/contracts/Limbo.sol#L606-L612
The private map state variable that has no getter (in solidity state variables are automatically private unless declared otherwise) https://github.com/code-423n4/2022-01-behodler/blob/main/contracts/Limbo.sol#L288
Manual code review
If you insist changing the allowance to a fixed number and not increase it or decrease it, at least make the allowance public so it can be checked before changing
#0 - gititGoro
2022-01-31T23:40:06Z
The suggestion to make allowance public is a good one. I think this was an accidental omission.
🌟 Selected for report: csanuragjain
Also found by: CertoraInc, danb
178.9074 USDC - $178.91
CertoraInc
_unstake() function)a user can't unstake his funds if he doesn't have a pending reward. it can make some problems when a user regrets and wants to withdraw his money straight away, so he doesn't have any pending reward and won't be able to withdraw his money (assuming we are on the same block timestamp).
#0 - gititGoro
2022-02-03T21:32:02Z
Technically you're right but flan is emitted per second. Given that blocks are minted at a rate lower than 1 block per second on mainnet, this edge case is only possible if flan per second is zero or if the withdraw happens in the same tx.
#1 - jack-the-pug
2022-02-27T09:54:11Z
Dup #210
CertoraInc
seed() function)save addresses like sushiLPs[i] and uniLPs[i] to save gas (saves the access to array elements)
code before:
for (uint256 i = 0; i < sushiLPs.length; i++) { require(UniPairLike(sushiLPs[i]).factory() == sushiFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(sushiLPs[i]) > 1000) assetApproved[sushiLPs[i]] = true; fateGrowthStrategy[sushiLPs[i]] = FateGrowthStrategy.indirectTwoRootEye; } for (uint256 i = 0; i < uniLPs.length; i++) { require(UniPairLike(uniLPs[i]).factory() == uniFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(uniLPs[i]) > 1000) assetApproved[uniLPs[i]] = true; fateGrowthStrategy[uniLPs[i]] = FateGrowthStrategy.indirectTwoRootEye; }
code after:
address temp; for (uint256 i = 0; i < sushiLPs.length; i++) { temp = sushiLPs[i]; require(UniPairLike(temp).factory() == sushiFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(temp) > 1000) assetApproved[temp] = true; fateGrowthStrategy[temp] = FateGrowthStrategy.indirectTwoRootEye; } for (uint256 i = 0; i < uniLPs.length; i++) { temp = uniLPs[i]; require(UniPairLike(temp).factory() == uniFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(temp) > 1000) assetApproved[temp] = true; fateGrowthStrategy[temp] = FateGrowthStrategy.indirectTwoRootEye; }
#0 - gititGoro
2022-02-10T00:07:57Z
duplicate of issue 12
#1 - jack-the-pug
2022-02-22T14:56:00Z
Dup #12
🌟 Selected for report: Dravee
Also found by: CertoraInc, Ruhum, hyh
31.8892 USDC - $31.89
CertoraInc
generateFLNQuote() function)can save latestFlanQuotes[0] in a local variable instead of accessing it multiple times (this saves some gas)
code before:
function generateFLNQuote() public override { latestFlanQuotes[1] = latestFlanQuotes[0]; ( latestFlanQuotes[0].DaiScxSpotPrice, latestFlanQuotes[0].DaiBalanceOnBehodler ) = getLatestFLNQuote(); latestFlanQuotes[0].blockProduced = block.number; }
code after:
function generateFLNQuote() public override { FlanQuote storage temp = latestFlanQuotes[0]; latestFlanQuotes[1] = temp; ( temp.DaiScxSpotPrice, temp.DaiBalanceOnBehodler ) = getLatestFLNQuote(); temp.blockProduced = block.number; }
#0 - jack-the-pug
2022-02-16T07:14:28Z
Dup with #336
🌟 Selected for report: Ruhum
Also found by: 0v3rf10w, CertoraInc, Dravee, camden, gzeon, hyh, sirhashalot
10.4613 USDC - $10.46
CertoraInc
purchasePyroFlan() function)calling redeemRate multiple times instead of one time (the first call is useless for sure)
redeemRate = PyroTokenLike(config.pyroFlan).redeemRate(); redeemRate = PyroTokenLike(config.pyroFlan).redeemRate(); // ... redeemRate = PyroTokenLike(config.pyroFlan).redeemRate(); PyroTokenLike(config.pyroFlan).mint(msg.sender, flanToMint + premium); redeemRate = PyroTokenLike(config.pyroFlan).redeemRate();
#0 - gititGoro
2022-02-09T23:34:21Z
duplicate of issue 79
🌟 Selected for report: CertoraInc
47.2433 USDC - $47.24
CertoraInc
constructor() function)inline the setDAO function code instead of calling it and reduce the checks (the require is not needed)
#0 - gititGoro
2022-02-03T21:15:06Z
This gas cost won't be borne by a user.
CertoraInc
seed() function)use ++i instead of i++
code before:
for (uint256 i = 0; i < sushiLPs.length; i++) { require(UniPairLike(sushiLPs[i]).factory() == sushiFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(sushiLPs[i]) > 1000) assetApproved[sushiLPs[i]] = true; fateGrowthStrategy[sushiLPs[i]] = FateGrowthStrategy.indirectTwoRootEye; } for (uint256 i = 0; i < uniLPs.length; i++) { require(UniPairLike(uniLPs[i]).factory() == uniFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(uniLPs[i]) > 1000) assetApproved[uniLPs[i]] = true; fateGrowthStrategy[uniLPs[i]] = FateGrowthStrategy.indirectTwoRootEye; }
code after:
for (uint256 i = 0; i < sushiLPs.length; ++i) { require(UniPairLike(sushiLPs[i]).factory() == sushiFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(sushiLPs[i]) > 1000) assetApproved[sushiLPs[i]] = true; fateGrowthStrategy[sushiLPs[i]] = FateGrowthStrategy.indirectTwoRootEye; } for (uint256 i = 0; i < uniLPs.length; ++i) { require(UniPairLike(uniLPs[i]).factory() == uniFactory, "LimboDAO: invalid Sushi LP"); if (IERC20(eye).balanceOf(uniLPs[i]) > 1000) assetApproved[uniLPs[i]] = true; fateGrowthStrategy[uniLPs[i]] = FateGrowthStrategy.indirectTwoRootEye; }
#0 - gititGoro
2022-02-09T23:59:37Z
duplicate of issue 10
#1 - jack-the-pug
2022-02-22T14:52:37Z
Dup #10
🌟 Selected for report: Dravee
Also found by: CertoraInc, defsec, pauliax, sirhashalot
22.9602 USDC - $22.96
CertoraInc
stabilizeFlan() function)can use unchecked to save gas spent on overflow checks
code before:
if (existingFlanOnLP < DesiredFinalFlanOnLP) { uint256 flanToMint = ((DesiredFinalFlanOnLP - existingFlanOnLP) * (100 - VARS.priceBoostOvershoot)) / 100; flanToMint = flanToMint == 0 ? DesiredFinalFlanOnLP - existingFlanOnLP : flanToMint; FlanLike(VARS.flan).mint(pair, flanToMint); IERC20(VARS.behodler).transfer(pair, rectangleOfFairness); { lpMinted = VARS.Flan_SCX_tokenPair.mint(VARS.blackHole); } }
code after:
if (existingFlanOnLP < DesiredFinalFlanOnLP) { uint256 flanToMint = (( unchecked { DesiredFinalFlanOnLP - existingFlanOnLP } ) * (100 - VARS.priceBoostOvershoot)) / 100; flanToMint = flanToMint == 0 ? unchecked { DesiredFinalFlanOnLP - existingFlanOnLP } : flanToMint; FlanLike(VARS.flan).mint(pair, flanToMint); IERC20(VARS.behodler).transfer(pair, rectangleOfFairness); { lpMinted = VARS.Flan_SCX_tokenPair.mint(VARS.blackHole); } }
#0 - gititGoro
2022-02-02T03:11:44Z
Gas consumption isn't as important in the migration as elsewhere.
#1 - jack-the-pug
2022-02-22T15:46:17Z
Dup #265
🌟 Selected for report: CertoraInc
174.9751 USDC - $174.98
CertoraInc
_governanceApproved() function)no need for successfulProposal variable
#0 - gititGoro
2022-02-03T21:13:37Z
The gas savings is low enough that I'd suggest this be reported as a code quality issue. Having the variable is a bit clumsy looking.
🌟 Selected for report: Ruhum
Also found by: CertoraInc, sirhashalot, ye0lde
31.8892 USDC - $31.89
CertoraInc
configure() function)the expression VARS.precision = precision == 0 ? precision : precision can be simply replaced by VARS.precision = precision, because the value of it is precision anyway (the condition is unnecessary).
#0 - gititGoro
2022-02-09T23:26:52Z
duplicate of issue 89
#1 - jack-the-pug
2022-02-27T12:32:19Z
Dup #89
🌟 Selected for report: CertoraInc
174.9751 USDC - $174.98
CertoraInc
constructor())use type(uint256).max instead of calculating 2**256-1
#0 - gititGoro
2022-02-02T04:09:10Z
The gas impact is the same. You're right from a code quality point of view. You should probably have logged this as a code quality suggestion.
#1 - gititGoro
2022-07-04T14:43:53Z
17.2202 USDC - $17.22
CertoraInc
It is more efficient to save the proposals' parameters as variables and not as struct variables, because of the address calculating when accessing the struct members.
For example when trying to access the y member of a coordinate struct variable
struct coordinate { uint256 x; uint256 y; } coordinate p = coordinate({x: 5, y:5}); p.y = 10;
it will take the address of p and add size of uint256 to it to calculate the address of p.y. If we had defined p without the struct (something like this: uint256 p_x = 5, p_y = 5;) it would have been more efficient because we don't need to calculate the address, we just take the address of p_y.
#0 - gititGoro
2022-02-03T22:30:48Z
Minor gas savings in proposals are not what we're looking for.
#1 - jack-the-pug
2022-02-27T12:03:43Z
Dup #222
🌟 Selected for report: BouSalman
Also found by: CertoraInc, Dravee
47.2433 USDC - $47.24
CertoraInc
purchasePyroFlan() function)dividing by 2 and by 4 can be done by shifting (x / 2 == x >> 1 and x / 4 == x >> 2) which consumes less gas
#0 - gititGoro
2022-02-09T23:21:14Z
duplicate of issue 95
#1 - jack-the-pug
2022-02-27T12:13:26Z
Dup #95
🌟 Selected for report: CertoraInc
174.9751 USDC - $174.98
CertoraInc
_ensurePriceStability() function)you save the storage array localFlanQuotes to a local (memory) array, but it is more efficient to save it's 2 elements to 2 local variables (this saves the calculating of the address by the index of the array's element and saves some gas)
code before:
FlanQuote[2] memory localFlanQuotes; //save gas localFlanQuotes[0] = latestFlanQuotes[0]; localFlanQuotes[1] = latestFlanQuotes[1];
code after:
FlanQuote memory localFlanQuotes0, localFlanQuotes1; localFlanQuotes0 = latestFlanQuotes[0]; localFlanQuotes1 = latestFlanQuotes[1];
🌟 Selected for report: CertoraInc
174.9751 USDC - $174.98
CertoraInc
buyFlanAndBurn() function)can save some gas by not evaluating the same expression twice
code before:
uint256 amount0Out = inputToken < VARS.flan ? 0 : amountOut; uint256 amount1Out = inputToken < VARS.flan ? amountOut : 0;
code after:
uint256 amount0Out, amount1Out; if (inputToken < VARS.flan) { amount0Out = 0; amount1Out = amountOut; } else { amount0Out = amountOut; amount1Out = 0; }