Missing events and sanity/threshold checks for critical onlyOwner setter functions in Liquidation

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

Lack of input validation on key function parameters is a best-practice. Not applying sanity/threshold checks will allow incorrect values to be set and affect the liquidation dynamics of the markets. Lack of events reduces transparency and ability for off-chain users/interfaces/monitors to evaluate and react to onchain changes.

Impact: Owner changes releaseTime, minimumLeftoverGasCostMultiplier or maxSlippage parameters of Liquidation. Without sanity/threshold checks or event emissions, these are changed to absurd values. Liquidations are significantly impacted. Users lose trust in markets and exit. Protocol reputation takes a hit.

Proof of Concept

https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/Liquidation.sol#L449-L472

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add reasonable sanity/threshold checks and event emissions to critical onlyOwner setter functions in Liquidation.

[raymogg]

Disagree with severity purely due to the fact that even if we were to set sanity thresholds, the ability for the owner to change these params still exists. As such, any threshold we set, while helpful to prevent ridiculous values, is still arbitrarily picked.

The lack of events is valid, however again probably not a medium risk as events are purely for UI's and don't affect the underlying operation of the market. Ideally UIs simply read the current set state variable over relying on events for these params. The point on reputation risk is good though. As such it's probably more of a low risk.

[cemozerr]

Marking this medium risk as not having a threshold check does allow owner to set to absurd values that might be detrimental to users funds.

[loudoguno]

labeling as duplicate of issue #64 and closing as per judges sheet