search

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

1 stars 0 forks source link

Missing sanity/threshold checks on critical contract parameter initializations #173

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

0xRajeev

Vulnerability details

Impact

Critical contract parameters of non-address types, i,e. integers should have sanity/threshold checks in constructor/initialize/setter to ensure they are relevant from the application-logic’s perspective. Accidentally setting arbitrary values may lead to reverts or financial loss by transferring/withholding more rewards/fees than specified.

In the setters of minLicenseFee, auctionDecrement, auctionMultiplier, bondPercentDiv or ownerSplit, only the setOwnerSplit() setter has a threshold check against a maximum of 20%. None of the others have any checks.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L39-L41

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L43-L45

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L47-L49

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L51-L53

https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L55-L59

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add sanity/threshold checks in constructor/initialize/setter for all critical contract parameters of non-address types.

GalloDaSballo commented 2 years ago

Agree with the finding because some of these can hinder the functionality of the protocol, additionally adding boundaries gives more well defined security guarantees to the users