code-423n4 / 2021-09-defiprotocol-findings

1 stars 0 forks source link

Sanity checks when the contract parameters are updated #234

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

hrkrshnn

Vulnerability details

Sanity checks when the contract parameters are updated

The protocol defines several parameters, for example, minLicenseFee which can be updated by using a corresponding setter function that the owner specifies.

However, there are no sanity checks for these parameters in the setter functions. Without sanity checks, the owner can set the parameters in a way to sabotage the protocol.

For example, setting auctionDecrement to 0 would mean that settleAuction function will always revert, making some operations in the protocol to halt. Therefore, adding a check that the value is non-zero in the setter avoids potential problems of this sort.

Recommended Mitigation Steps

For each parameter, add a require check for both upper and lower-bounds.

frank-beard commented 2 years ago

it is assumed the owner is trusted in this version

GalloDaSballo commented 2 years ago

Personally I believe that adding checks to limit admin privileges is a way to gain community trust, as well as properly avoid potential rug vectors This typically helps users of the protocols (as the protocol doesn't require trust in the owner / dev) as well as avoid potential mistakes

I think the finding is valid, and while superficially trivial (why would I set it to the wrong value), I highly recommend the sponsor to add checks as they are security guarantees to the users of the protocol