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

1 stars 0 forks source link

Missing timelocks for critical protocol parameter setters by owner
 #172

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

0xRajeev

Vulnerability details

Impact

While publishers are enforced a one day timelock for changePublisher() and changeLicenseFee() functions, Factory owner setter functions should also have timelocks (along with events) so that users and other privileged roles (publishers) can detect upcoming changes and have the time to react to them.

Changes to minLicenseFee, auctionDecrement, auctionMultiplier, bondPercentDiv or ownerSplit may have a financial or trust impact on users/publishers who should be given an opportunity to react to them by exiting/engaging without being surprised when such changes are made effective immediately.

None of the onlyOwner setters in the protocol have a timelock controller functionality to delay enforcement of the critical changes they make.

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

Consider adding timelocks to such contracts with critical setter functions.

GalloDaSballo commented 2 years ago

This is nuanced, as the invariant of having a timelock could be enforced at the governance / owner level. However, since there's an implementation of timelock for other parameters, I do agree that these parameters should be held by the same standard