Platform: Code4rena
Start Date: 02/04/2021
Pot Size: $50,000 USDC
Total HM: 20
Participants: 5
Period: 6 days
Judge: Zak Cole
Total Solo HM: 19
Id: 3
League: ETH
Rank: 2/5
Findings: 7
Award: $18,227.61
🌟 Selected for report: 16
🚀 Solo Findings: 5
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
It is unclear if the function applyInterest is supposed to return a new balance with the interest applied or only the accrued interest? There are various usages of it, some calls add the return value to the old amount: return bond.amount + applyInterest(bond.amount, cumulativeYield, yieldQuotientFP); and some not:
balanceWithInterest = applyInterest( balance, yA.accumulatorFP, yieldQuotientFP );
This makes the code misbehave and return the wrong values for the balance and accrued interest.
Make it consistent in all cases when calling this function.
#0 - werg
2021-04-08T21:21:44Z
This is correct and has been fixed in the core repo
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
function buyBond transfers amount from msg.sender twice: Fund(fund()).depositFor(msg.sender, issuer, amount); ... collectToken(issuer, msg.sender, amount);
This makes the msg.sender pay twice for the same bond.
Charge poor man only once.
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
uint256 public diffMaxMinRuntime; This variable is never set nor updated so it gets a default value of 0.
diffMaxMinRuntime with 0 value is making the calculations that use it either always return 0 (when multiplying) or fail (when dividing) when calculating bucket indexes or sizes.
Set the appropriate value for diffMaxMinRuntime and update it whenever min or max runtime variables change.
🌟 Selected for report: pauliax
getPriceFromAMM relies on values returned from getAmountsOut which can be manipulated (e.g. with the large capital or the help of flash loans). The impact is reduced with UPDATE_MIN_PEG_AMOUNT and UPDATE_MAX_PEG_AMOUNT, however, it is not entirely eliminated.
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
Uniswap v2 recommends using their TWAP oracle: https://uniswap.org/docs/v2/core-concepts/oracles/
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
CrossMarginTrading sets value of liquidationThresholdPercent in the constructor: liquidationThresholdPercent = 110; Isolated margin contracts declare but do not set the value of liquidationThresholdPercent.
Set the initial value for the liquidationThresholdPercent in Isolated margin contracts.
This makes function belowMaintenanceThreshold to always return true unless a value is set via function setLiquidationThresholdPercent. Comments indicate that the value should also be set to 110:
// The following should hold: // holdings / loan >= 1.1 // => holdings >= loan * 1.1
only admin can call this so highly unlikely to happen yet it would be better if code prevents that.
require share to be greater than 0.
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
function initTranche should check that the "share" parameter is > 0, otherwise, it may be possible to initialize the same tranche again.
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
Here, the revert message says that the value needs to be at least 1 hour, however, the code allows value only above the 1 hour (> instead of >=): require(runtime > 1 hours, "Min runtime needs to be at least 1 hour");
no impact on security, just a discrepancy between the check and message.
Replace > with >= or update the error message to reflect that.
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
function setLeveragePercent should check that the _leveragePercent >= 100 so that this calculation will not fail later: (leveragePercent - 100)
This variable can only be set by admin so as long as he sets the appropriate value it should be fine.
It is always nice to enforce such things via code. Code is law they say.
#0 - werg
2021-04-09T02:06:52Z
thanks, but in this case that would be governance's job to check
🌟 Selected for report: pauliax
Gas optimization suggestion:
If you want to reduce the deployment costs, consider using error codes as error messages. Now on revert, it returns long messages like "Contract not authorized to deposit for user". Longer messages need more space so a possible optimization is to store error codes (e.g. "F1") and map them with messages on the UI part.
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
🌟 Selected for report: pauliax
Gas optimization suggestion:
contract CrossMarginLiquidation, here the same calculations are done twice: (peg2targetCost * (100 + MAINTAINER_CUT_PERCENT)) / 100 it would be better to extract it into a variable and use it instead. Same here: emit LiquidationShortfall(liquidationTarget - liquidationReturns); Lending(lending()).haircut(liquidationTarget - liquidationReturns);
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
Gas optimization suggestion:
Contract RoleAware has a role that is not used anywhere: uint256 constant TOKEN_ADMIN = 109; contract IsolatedMarginTrading has variables that are not used anywhere:
/// update window in blocks uint16 public priceUpdateWindow = 8; uint256 public UPDATE_RATE_PERMIL = 80; Variables with the same name are declared and used in contract PriceAware.
Contract IsolatedMarginAccounts has a variable that is not used anywhere: coolingOffPeriod. Variable with the same name is used in CrossMarginTrading.
Another not used variable: uint256 public borrowingMarkupFP;
#0 - werg
2021-04-09T00:58:29Z
TOKEN_ADMIN is used in other parts of the codebase related to initialization, not presented here.
🌟 Selected for report: pauliax
Gas optimization suggestion:
There are places that do not check if the amount is greater than 0. If the amount is 0 it still performs useless calculations (e.g. adding 0 to the total balance, etc). An example where checking against 0 could save some gas (calls _registerDeposit even when addedHolding is 0): // no overflow because depositAmount >= extinguishableDebt uint256 addedHolding = depositAmount - extinguishableDebt; _registerDeposit(account, token, addedHolding); or here function _withdrawBond may return 0 when there is a liquidity issue:
uint256 withdrawAmount = super._withdrawBond(bondId, bond); disburse(bond.issuer, msg.sender, withdrawAmount);
0x523B5b2Cc58A818667C22c862930B141f85d49DD
paulius.eth
🌟 Selected for report: pauliax
Gas optimization suggestion:
Not used imports: Contracts CrossMarginAccounts and CrossMarginTrading import "./MarginRouter.sol"; but do not use it. Contract Admin import "./CrossMarginTrading.sol"; Also unused imports in several contracts: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/access/Ownable.sol";
0x523B5b2Cc58A818667C22c862930B141f85d49DD
paulius.eth
🌟 Selected for report: pauliax
paulius.eth
0x523B5b2Cc58A818667C22c862930B141f85d49DD
Gas optimization suggestion:
Storage access is expensive. holdingTokens length can be extracted into a variable and used where necessary to reduce the number of storage read. change this: holdingTokens = account.holdingTokens; holdingAmounts = new uint256; for (uint256 idx = 0; holdingTokens.length > idx; idx++) to this: holdingTokens = account.holdingTokens; uint holdingTokensLength = holdingTokens.length; holdingAmounts = new uint256; for (uint256 idx = 0; holdingTokensLength > idx; idx++)
🌟 Selected for report: pauliax
Gas optimization suggestion:
Useless addition of 0 here: return bond.amount + 0;
0x523B5b2Cc58A818667C22c862930B141f85d49DD
paulius.eth
🌟 Selected for report: pauliax
Gas optimization suggestion:
Here it is better to replace ">=" with ">" as sending 0 is a waste of gas (similar code is also used in CrossMarginLiquidation): // send back trader money if (holdingsValue >= maintainerCut4Account + account.borrowed) { // send remaining funds back to trader Fund(fund()).withdraw( borrowToken, traderAddress, holdingsValue - account.borrowed - maintainerCut4Account ); }
0x523B5b2Cc58A818667C22c862930B141f85d49DD
paulius.eth