Timeswap contest - Beepidibop's results
Like Uniswap, but for lending & borrowing.
General Information
Platform: Code4rena
Start Date: 20/01/2023
Pot Size: $90,500 USDC
Total HM: 10
Participants: 59
Period: 7 days
Judge: Picodes
Total Solo HM: 4
Id: 206
League: ETH
Findings Distribution
Researcher Performance
Rank: 8/59
Findings: 2
Award: $3,251.16
🌟 Selected for report: 0
🚀 Solo Findings: 0
Findings Information
🌟 Selected for report: chaduke
Also found by: 0xcm, Beepidibop
Labels
Awards
3202.6177 USDC - $3,202.62
External Links
Lines of code
Vulnerability details
Proof of Concept
collect() uses the wrong paramenters when calling ITimeswapV2Pool.transferFees(). It uses long0Fees, long1Fees, and shortFees instead of param.long0FeesDesired, param.long1FeesDesired, and param.shortFeesDesired. The former 3 are defined in the function's returns paramenters and aren't mutated before calling ITimeswapV2Pool.transferFees(). They all default to zero which would trigger a revert since TimeswapV2Pool.transferFees() reverts when 0 fees are being transferred on line 167
Code snippet where the parameters default to 0
Code snippet where transferFees() revert
Impact
This means users who mints liquidity tokens will be forfeiting their fees since collect() will always be calling ITimeswapV2Pool.transferFees() with 0s as inputs. So I think a medium risk is warranted since users are losing funds.
Recommendation
Switch to param.long0FeesDesired, param.long1FeesDesired, and param.shortFeesDesired and check that the msg.sender has enough fees to be transferred.
OR
Place line 193 after line 199, which does the check and mutates (long0Fees, long1Fees, shortFees) for you
#0 - c4-judge
2023-02-01T09:49:27Z
Picodes marked the issue as duplicate of #121
#1 - c4-judge
2023-02-12T22:27:34Z
Picodes changed the severity to 3 (High Risk)
#2 - c4-judge
2023-02-12T22:27:40Z
Picodes marked the issue as satisfactory
Findings Information
🌟 Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0xackermann, Aymen0909, Beepidibop, IllIllI, Iurii3, Rageur, RaymondFam, ReyAdmirado, Rolezn, SaeedAlipoor01988, Udsen, Viktor_Cortess, W0RR1O, W_Max, atharvasama, c3phas, chaduke, descharre, fatherOfBlocks, kaden, matrix_0wl, shark
Awards
48.5424 USDC - $48.54
Labels
External Links
Gas Findings
[GAS-1] ReentrancyGuard: Consider deprecating NOT_INTERACTED and changeInteractedIfNecessary
Consider deprecating checking for NOT_INTERACTED in ReentrancyGuard.check since actions such as mint/burn that require ReentrancyGuard.check() will revert due to division by zero or underflows and collecting fees would collect 0 if nothing was minted in the first place.
Places that use ReentrancyGuard.check and what stops it from being necessary
| Contract | Function | Reason |
|---|---|---|
TimeswapV2Pool | collectProtocolFees() | Would collect 0 fees |
TimeswapV2Pool | collectTransactionFees() | Would collect 0 fees |
TimeswapV2Pool | mint() | Reverts on divide by 0 here (pool.sqrtInterestRate is 0) |
TimeswapV2Pool | burn() | Reverts on hasLiquidity() |
TimeswapV2Pool | deleverage() | Reverts on hasLiquidity() |
TimeswapV2Pool | leverage() | Reverts on hasLiquidity() |
TimeswapV2Pool | rebalance() | Reverts on hasLiquidity() |
TimeswapV2LiquidityToken | mint() | changeInteractedIfNecessary() in the line right before, which means minting without needing interaction is intended behaviour |
TimeswapV2LiquidityToken | burn() | Reverts on underflow |
TimeswapV2LiquidityToken | collect() | Transfers 0 fees even if collect() is properly implemented ([M-1] TimeswapV2LiquidityToken: collect() will always revert because it uses the wrong paramenters when calling ITimeswapV2Pool.transferFees()) |
TimeswapV2Token | mint() | changeInteractedIfNecessary() in the line right before, which means minting without needing interaction is intended behaviour |
TimeswapV2Token | burn() | Reverts on underflow |
Recommendation
Consider deprecating checking for NOT_INTERACTED in ReentrancyGuard.check and changeInteractedIfNecessary.
Link To Affected Code
[GAS-2] PoolLibrary: Consider putting pool.xProtocolFees into memory before comparison in collectProtocolFees()
Putting pool.xProtocolFees in memory before the if statement for the 3 types of fees can save gas on SLOADing the same slot multiple times.
Recommendation
Consider putting pool.xProtocolFees into memory before comparison in collectProtocolFees()
Link To Affected Code
#0 - c4-judge
2023-02-02T12:29:06Z
Picodes marked the issue as grade-b