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: 3/5
Findings: 5
Award: $6,017.02
🌟 Selected for report: 8
🚀 Solo Findings: 3
gpersoon
gpersoon.eth
The variable lastUpdatedDay in IncentiveDistribution.sol is not (properly) initialized. This means the function updateDayTotals will end up in a very large loop which will lead to an out of gas error. Even if the loop would end, the variable currentDailyDistribution would be updated very often. Thus updateDayTotals cannot be performed
The entire IncentiveDistribution does not work. If the loop would stop, the variable currentDailyDistribution is not accurate, resulting in a far lower incentive distribution than expected.
Initialize lastUpdatedDay with something like block.timestamp / (1 days)
uint256 lastUpdatedDay; # ==> lastUpdatedDay = 0
#When the function updateDayTotals is called: uint256 public nowDay = block.timestamp / (1 days); #==> ~ 18721 uint256 dayDiff = nowDay - lastUpdatedDay; #==> 18721-0 = 18721
for (uint256 i = 0; i < dayDiff; i++) { # very long loop (18721) currentDailyDistribution = .... } #will result in an out of gas error
🌟 Selected for report: gpersoon
gpersoon
gpersoon.eth
The functions crossSwapTokensForExactTokens and crossSwapExactTokensForTokens of MarginRouter.sol do not check who is calling the function. They also do not check the contents of pairs and tokens They also do not check if the size of pairs and tokens is the same
registerTradeAndBorrow within registerTrade does seem to do an entry check (require(isMarginTrader(msg.sender)...) however as this is an external function msg.sender is the address of MarginRouter.sol, which will verify ok.
Calling these functions allow the caller to trade on behalf of marginswap, which could result in losing funds. It's possible to construct all parameters to circumvent the checks. Also the "pairs" can be fully specified; they are contract addresses that are called from getAmountsIn / getAmountsOut and from pair.swap. This way you can call arbitrary (self constructed) code, which can reentrantly call the marginswap code.
Based on source code review. A real attack requires the deployed code to be able to construct the right values.
remix
Limit who can call the functions Perhaps whitelist contents of pairs and tokens Check the size of pairs and tokens is the same
#0 - werg
2021-04-04T15:01:38Z
This has merit: particularly the part about self-constructed pairs. We either need much more rigorous checks or a a process for vetting & approving pairs. The latter is likely more gas efficient.
🌟 Selected for report: gpersoon
gpersoon
gpersoon.eth
The function liquidate (in both CrossMarginLiquidation.sol and IsolatedMarginLiquidation.sol) can be called by everyone. If an attacker calls this repeatedly then the maintainer will be punished and eventually be reported as maintainerIsFailing And then the attacker can take the payouts
When a non authorized address repeatedly calls liquidate then the following happens: isAuthorized = false which means maintenanceFailures[currentMaintainer] increases after sufficient calls it will be higher than the threshold and then maintainerIsFailing() will be true This results in canTakeNow being true which finally means the following will be executed: Fund(fund()).withdraw(PriceAware.peg, msg.sender, maintainerCut);
An attacker can push out a maintainer and take over the liquidation revenues
remix
put authorization on who can call the liquidate function review the maintainer punishment scheme
#0 - werg
2021-04-04T14:41:34Z
I believe this issue is not a vulnerability, due to the checks in lines 326-335. Even if someone comes in first and claims the maintainer is failing they can do their job in the same or next block and get all / most of their failure record extinguished.
#1 - zscole
2021-04-21T14:24:01Z
Acknowledging feedback from @werg, but maintaining the reported risk level of medium since this has implications on token logic.
🌟 Selected for report: gpersoon
gpersoon
gpersoon.eth
The following functions have no entry check or a trivial entry check: withdrawHourlyBond Lending.sol closeHourlyBondAccount Lending.sol haircut Lending.sol addDelegate(own adress...) Admin.sol removeDelegate(own adress...) Admin.sol depositStake Admin.sol disburseLiqStakeAttacks CrossMarginLiquidation.sol disburseLiqStakeAttacks IsolatedMarginLiquidation.sol getCurrentPriceInPeg PriceAware.sol
By manipulating the input values (for example extremely large values) you might be able to disturb the internal administration of the contract, thus perhaps locking function or giving wrong rates.
note: function haircut is trivial so hardly any risk
Check the functions to see if they are completely risk free and add entry checks if they are not. Add a comment to notify the function is meant to be called by everyone.
Based on source code review. A real attack requires the deployed code to be able to construct the right values.
#0 - werg
2021-04-04T14:32:05Z
withdrawHourlyBond: could not find vulnerability, since solidity 0.8.x fails on underflow in HourlyBondSubscriptionLending.sol:115 in case of unauthorized access.closeHourlyBondAccount: same story since both call into _withdrawHourlyBondhaircut: trivially guarded in one way, though this actually has merit in another way -- if at some point down the road an attacker were able to establish a token, make it popular enough for us to add it to cross margin, but include in that token contract a malicious function that calls haircut, they could then void everybody's bonds in their token. I don't see how it would be profitable, it's definitely an expensive long con, but... we should add an extra guard to make sure it's an isolated margin trading contract.addDelegate has a guard.removeDelegate has a guard as well, or am I missing something here?depositStake fails for unfunded requests in the safe transfer in Fund.depositFordisburseLiqStakeAttacks should be universally accessible by designgetCurrentPriceInPeg only updates state in a rate limited way, hence fine for it to be publicI will add comments to the effect. Thanks again
🌟 Selected for report: gpersoon
gpersoon
gpersoon.eth
This is a minor suggestion.
Roles.sol contains the following: roles[msg.sender][9] = true; It's not clear what the number 9 means. In RoleAware.sol there is a constant with the value 9: uint256 constant TOKEN_ACTIVATOR = 9;
The code is more difficult to read without an explanation for the number 9. In case the code would be refactored in the future and the constants in RoleAware.sol are renumbered, the value in Roles.sol would no longer correspond to the right value.
Move the constants from Roles.sol to RoleAware.sol and replace 9 with the appropriate constant.
#0 - werg
2021-04-04T13:58:26Z
Yes! I recently saw this as well and already fixed it in our code base 😉
🌟 Selected for report: gpersoon
gpersoon
gpersoon.eth
The function liquidate, which is defined in both CrossMarginLiquidation.sol and IsolatedMarginLiquidation.sol, includes the modifier noIntermediary. This modifier prevents the use of Multisig wallets.
If the maintainer happens to use a multisig wallet he might not experience any issues until he tries to call the function liquidate. At that moment he can't successfully call the function.
Verify if the prevention to use multisig wallets is intentional. In that case add a comment to the liquidate functions. If it is not intentional update the code so multisigs wallets can be supported.
🌟 Selected for report: gpersoon
gpersoon
gpersoon.eth
The solidity version in UniswapStyleLib.sol (>=0.5.0) is different than the solidity version in the other contracts (e.g. ^0.8.0) Also math actions are present in the functions getAmountOut and getAmountIn that could easily lead to an underflow or division by 0; (note safemath is not used). Note: In solidity 0.8.0 safemath like protections are default.
The impact is low because UniswapStyleLib is a library and the solidity version of the contract that uses the library is used (e.g. ^0.8.0), which has safemath like protections. It is cleaner to have the same solidity version everywhere.
getAmountIn(3,1,1000) would give division by 0 getAmountIn(1,1,1) will underflow denominator
Use the same solidity version everywhere
🌟 Selected for report: gpersoon
this is a minor suggestion:
The function sortTokens UniswapStyleLib.sol returns 2 values, but only the first return value is used: MarginRouter.sol: (address token0, ) = UniswapStyleLib.sortTokens... UniswapStyleLib.sol: (address token0, ) = sortTokens.. In both cases the used return value is compared to the first parameter of the function call. Conclusion: the function is only used to determine the smaller of the two tokens, not really to sort tokens.
gpersoon
gpersoon.eth
The code is somewhat more difficult to read and a bit longer than neccesary.
simplify the code: function ASmallerThanB(address tokenA, address tokenB) internal pure returns (bool) { require(tokenA != tokenB, "Identical address!"); require(tokenA != address(0), "Zero address!"); require(tokenB != address(0), "Zero address!"); return tokenA < tokenB; }