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

0 stars 0 forks source link

Missing cap on LicenseFee #154

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

gzeon

Vulnerability details

Impact

There is no cap on LicenseFee. While change of LicenseFee is under 1 day timelock, introducing a
maxLicenseFee can improve credibility by removing the "rug" vector. There is a minLicenseFee in the contracts, while imo make little sense to have minLicenseFee but not maxLicenseFee.

An incorrectly set LicenseFee can potentially lead to over/underflow in https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L140-141 which is used in most of the function.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L177 https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Factory.sol#L77 https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L49

Recommended Mitigation Steps

Define a maxLicenseFee

frank-beard commented 2 years ago

Generally it is intended for the publishers to act correctly and the timelock is intended to prevent incorrect values from making it all the way through, however there is validity in reducing how the fee can be modified, such as reducing how much any one fee change can change the fee. I would consider this a low risk issue

0xleastwood commented 2 years ago

It seems like changes to licenseFee could potentially brick the contract as handleFees() underflows, preventing users from minting/burning tokens. I'd deem this as medium severity due to compromised protocol availability.