search

code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

Add max fee in setFee and emit event #289

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L119-L121

Vulnerability details

Impact

Malicious owner can steal all ETH of a sell.

Proof of Concept

The function setFee#CallyNFT.sol is critical as it set the amount of ETH that the protocol will receive. A malicious owner can set the fee to 1e18 and all ETH after exercise will go to the owner of the contract. 

if (feeRate > 0) {                          #CallyNFT.sol#L288

 fee = (msg.value * feeRate) / 1e18;

  protocolUnclaimedFees += fee;

}

The seller (or beneficiary) will receive 0 ETH.

Recommended Mitigation Steps

In addition to the emitted event I would introduce a maximum fee to add transparency.

Ideally every time the fee changes it should only affect the new auction but not the previous. For example with a timelock. Although this requires much more work, maybe an idea could be to put fee as a parameter of the vault which is completed with the current fee.

outdoteth commented 2 years ago

Renamed issue to be more clear

outdoteth commented 2 years ago

owner can change fee at any time; https://github.com/code-423n4/2022-05-cally-findings/issues/47 owner can set fee greater than 100%: https://github.com/code-423n4/2022-05-cally-findings/issues/48