valorem-labs-inc / clear

Valorem Clear is a DeFi money lego, enabling writing covered calls and cash-secured puts, physically settled or cash settled, American, European, and Exotic options.
https://valorem.xyz/docs/clear-overview/
Other
89 stars 12 forks source link

fix write logic bug by simplifying write logic #130

Closed 0xAlcibiades closed 1 year ago

0xAlcibiades commented 1 year ago

This fixes an issue found during the Zellic audit by simplifying the write() function such that the condition can no longer occur.

From the auditors:

Q:

if (claimId != 0 && ((claimId >> 96) != (optionId >> 96))) {should the following scenario be reachable:

  1. call write with optionId == claimId.
  2. assume somehow we can get balanceOf[msg.sender][optionId] = 1 (optionId there since encodedClaimId = claimID = optionID)-> the function won't revert with CallerDoesNotOwnClaimId(encodedClaimId);.
  3. update a _claim[encodedClaimId]; that you shouldn't have

If all these assumptions hold and are sane enough, it could lead to some locked funds.

A:

This is a valid finding, and we were able to reproduce, and it should not be allowed, herein, we propose a fix.

codecov-commenter commented 1 year ago

Codecov Report

Merging #130 (8d0e3c8) into master (6c118f2) will decrease coverage by 0.16%. The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   88.66%   88.49%   -0.17%     
==========================================
  Files           2        2              
  Lines         344      339       -5     
  Branches       54       52       -2     
==========================================
- Hits          305      300       -5     
  Misses         30       30              
  Partials        9        9              
Impacted Files Coverage Δ
src/OptionSettlementEngine.sol 99.05% <94.44%> (-0.03%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.