Platform: Code4rena
Start Date: 04/01/2022
Pot Size: $75,000 USDC
Total HM: 17
Participants: 33
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 74
League: ETH
Rank: 4/33
Findings: 3
Award: $4,577.76
🌟 Selected for report: 7
🚀 Solo Findings: 0
🌟 Selected for report: Rhynorater
Also found by: WatchPug, harleythedog, hyh
hyh
The collateral requirements for debt positions created during mint and borrow operations differ. When the quantity of the asset added is low compared to current asset holdings of a pool, a LP has clear incentives to repay its debt right after mint and borrow the same amount of the asset with lower collateral requirements.
This will leave its liquidity shares intact, as repaying the debt doesn't spend them. The collateral in the same time will be freed up due to borrowers’ requirements being less restrictive.
BorrowMath.getCollateral scales additional collateral requirements, making them less for the cases when xDecrease < state.x – xDecrease, i.e. for the case of non-whale borrower its collateral requirement is decreased by xDecrease / (state.x – xDecrease):
MintMath.getCollateral does not scale the collateral requirement, placing flat 1.0 coefficient in the same formula:
Consider bringing MintMath.getCollateral in line with BorrowMath.getCollateral for the low asset provision cases. For high asset provision cases it makes sense to avoid penalizing the LPs for bringing in more assets to the pool.
#0 - Mathepreneur
2022-01-24T12:51:27Z
Duplicate from #187
#1 - 0xean
2022-01-25T21:22:46Z
moving to sev-3 to match dupe.
🌟 Selected for report: hyh
567.9485 USDC - $567.95
hyh
Core configuration variables are immutable and are set in constructor in the most of system contracts, but aren't checked to be within desired bounds (for maturity and fees, for example) and non-zero (for convenience and pair, asset and collateral addresses).
If there be any operational level mistake in the deployment scripts the corresponding contracts will be instantiated, but will not be operational at best, so will have to be redeployed.
At worst there can be a malfunction due to misconfiguration down the road, in the case if this wouldn't be noticed early enough.
CollateralizedDebt: https://github.com/code-423n4/2022-01-timeswap/blob/main/Timeswap/Timeswap-V1-Convenience/contracts/CollateralizedDebt.sol#L66
TimeswapPair: https://github.com/code-423n4/2022-01-timeswap/blob/main/Timeswap/Timeswap-V1-Core/contracts/TimeswapPair.sol#L112
TimeswapFactory: https://github.com/code-423n4/2022-01-timeswap/blob/main/Timeswap/Timeswap-V1-Core/contracts/TimeswapFactory.sol#L39
Add sanity checks for all core variables in the constructors.
#0 - Mathepreneur
2022-01-18T16:31:54Z
Yes the restriction to those parameters will be the responsibility of the Convenience contract.
🌟 Selected for report: hyh
567.9485 USDC - $567.95
hyh
There can be issues with troubleshooting and system usage analytics.
E512 error code is meant for 'Debt is greater than max Debt' situation:
In Mint library E512 is used for collateral max value check:
Also, code E513, that is to be used for collateral check above, is also used for max asset increase check, which doesn’t seem to have an error code of its own:
Change Mint line 482 error code to be 513: https://github.com/code-423n4/2022-01-timeswap/blob/main/Timeswap/Timeswap-V1-Convenience/ErrorCodes.md#e513
Possibly add an error code for max asset check for Mint lines 481 and 643.
#0 - Mathepreneur
2022-01-17T20:10:17Z
🌟 Selected for report: hyh
567.9485 USDC - $567.95
hyh
Troubleshooting will be more complicated / not always possible whenever params structure is not filled properly as there will be low level reverts across the system without proper error messages.
For example, pay functionality has the following call sequence, starting with user facing repay function where its parameters are first entered:
TimeswapConvenience.repay(params) -> Pay.pay -> Pay._pay, where params.asset and params.collateral are used (without checks):
E501 will be thrown there if either address is zero, but its description doesn’t fully match the issue:
Then, Pay._pay packs ‘params’ to ‘data’, -> CollateralizedDebt.burn -> TimeswapPair.pay -> Callback.pay -> CollateralizedDebt.timeswapPayCallback -> TimeswapConvenience.collateralizedDebtCallback, where params.from is used (without checks):
Here, safeTransferFrom will fail if params.from is zero.
The same can be outlined for other usages of params structure in TimeswapConvenience:
Add the error messages and the non-zero checks for the components right before using each of them (in the pay example above: params.asset and params.collateral in Pay._pay, params.from in TimeswapConvenience.collateralizedDebtCallback).
#0 - Mathepreneur
2022-01-24T12:55:38Z
We will be careful on the implementation.
🌟 Selected for report: hyh
567.9485 USDC - $567.95
hyh
If there is no liquidity in the pool, burn operation will not make sense and be reverted on low level math of decreasing liquidity share. It will be more transparent and uniform to add a check for liquidity before the logic.
burn doesn't check for liquidity of the pool:
There is a check in other relevant functions, it can be added in the very same form here:
require(pool.state.totalLiquidity > 0, 'E206')
Error description can be updated to include burn:
#0 - Mathepreneur
2022-01-17T16:44:54Z
🌟 Selected for report: hyh
567.9485 USDC - $567.95
hyh
Similarly, the system will fail with low-level message without giving a business reason, which can be an issue for troubleshooting and further programmatic usages by other projects.
The owner variable is used by TimeswapPair.pay without validation:
Which will yield low-level fail on array access if an owner is zero or not present in the system:
Verify owner function argument to be non-zero by expanding existing check:
Then, require in line 358 that, for example, dues.length >= ids.length:
#0 - Mathepreneur
2022-01-17T16:42:55Z
🌟 Selected for report: hyh
567.9485 USDC - $567.95
hyh
System will fail with low-level message without giving a business reason, which can be an issue for troubleshooting and further programmatic usages by other projects.
If a borrower tries to get almost all available assets, the low level division can fail in the check function:
Add a maximum share of current assets that can be borrowed and revert with the corresponding error message before running the computations.
#0 - Mathepreneur
2022-01-24T12:56:23Z
It's designed such that borrowers cannot borrow up to that amount.
41.8864 USDC - $41.89
hyh
Block number is retrieved on each iteration, albeit being the same. Gas is overspent on calls involved.
Save block number to memory variable to use in the cycle.
#0 - amateur-dev
2022-01-15T04:23:58Z
Similar issue reported over here #142; hence closing this
🌟 Selected for report: hyh
93.0809 USDC - $93.08
hyh
Gas is overspent on state variables storage access.
getCollateral only reads state.reserves.asset, state.totalClaims.insurance and state.reserves.collateral up to 2 times each, and state.totalClaims.bond up to 4 times:
Save all four state variables to memory before running the logic.
#0 - Mathepreneur
2022-01-18T16:22:30Z