Several function have no entry check

[code423n4]

Email address

mail@gpersoon.com

Handle

gpersoon

Eth address

gpersoon.eth

Vulnerability details

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

Impact

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

Recommended mitigation steps

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.

Proof of concept

Based on source code review. A real attack requires the deployed code to be able to construct the right values.

[werg]

• 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 _withdrawHourlyBond
• haircut: 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.depositFor
• disburseLiqStakeAttacks should be universally accessible by design
• getCurrentPriceInPeg only updates state in a rate limited way, hence fine for it to be public

I will add comments to the effect. Thanks again