Skip to content

[ORE-05M] Potential Loss of Native Funds #512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Parent: v1.1.0 Migration + Simulation
merged 6 commits into from
May 14, 2025
Merged

[ORE-05M] Potential Loss of Native Funds #512

merged 6 commits into from
May 14, 2025

Conversation

@zajck
Copy link
Contributor

@zajck zajck commented May 9, 2025

Close #511

@zajck zajck self-assigned this May 9, 2025
@zajck zajck requested review from 0xlucian and levalleux-ludo May 9, 2025 11:34
0xlucian
0xlucian requested changes May 9, 2025
View reviewed changes
zajck and others added 2 commits May 9, 2025 13:48
@zajck @0xlucian
Update test/protocol/offerFacet.ts
7a55ec2 
Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>
@zajck @0xlucian
Update test/protocol/offerFacet.ts
383a143 
Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>
@zajck zajck requested a review from 0xlucian May 9, 2025 11:48
levalleux-ludo
levalleux-ludo reviewed May 12, 2025
View reviewed changes
contracts/protocol/facets/Offer.sol
handleBosonSellerDeposit(sellerId, exchangeToken, offer.sellerDeposit);

// WrapType wrapType = _wrapType;
if (_wrapType != FermionTypes.WrapType.SELF_SALE && offer.sellerDeposit == 0 && msg.value != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me why we exclude the case where offer.sellerDeposit != 0.
If an offer.exchangeToken is an ERC20, it doesn't make sense to send native funds on the transaction, so I think it should revert whatever the value of the sellerDeposit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When offer.sellerDeposit != 0, the checks are done in handleBosonSellerDeposit.

At least, they are when availableFunds < _sellerDeposit, but they were missing for the case availableFunds >= _sellerDeposit. I added it now, thanks for asking.

@zajck zajck requested a review from levalleux-ludo May 12, 2025 18:04
0xlucian
0xlucian requested changes May 14, 2025
View reviewed changes
@zajck @0xlucian
Update test/protocol/offerFacet.ts
bb3e4f6 
Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>
0xlucian
0xlucian approved these changes May 14, 2025
View reviewed changes
Copy link
Collaborator

@0xlucian 0xlucian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zajck zajck merged commit e9e764e into 343-v1.1.0-migration May 14, 2025
1 check passed
@zajck zajck deleted the 511-potential-loss-of-native-funds branch May 15, 2025 13:22
@zajck zajck mentioned this pull request May 15, 2025
Closed
zajck added a commit that referenced this pull request May 15, 2025
@0xlucian @zajck
v1.1.0 Migration + Simulation (#451)
c332d7a 
* feat: upgrade script, fork simulation

* chore: add instructions for upgrade and fork tests in README

* chore: working version of generate and upgrade suite (only missing upgrade-clients)

* fix: fork test and implement default replaceAll colliding selectors in fork tests

* refactor: CreatorToken interface rename

* revert: CreatorToken name back to original naming

* chore: add generation of clients diff in generate-upgrade-config

* refactor: finalize implementation of both scripts and refactor README

* fix: fork dryRun initialization

* refactor: add pause/unpause protocol and fix upgrade hook path

* refactor: throw error if hook fails

* refactor: address PR comments

* Update scripts/upgrade/upgrade-clients.ts

Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>

* fix: deploymentComplete function overwrites newly deployed contracts if they exist

* refactor: address PR comments

* Update scripts/upgrade/upgrade-clients.ts

Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>

* chore: add missing config files

* fix: re-add accidentally deleted tasks from hardhat config

* Sepolia v1.1.0 upgrade

* Amoy v1.1.0-rc.1 upgrade

* Base Sepolia v1.1.0-rc.1 upgrade

* fix: re-upgrade clients on Sepolia test

* Optimism Sepolia v1.1.0-rc.1 upgrade

* Arbitrum Sepolia v1.1.0-rc.1 upgrade

* refactor: fixing minor script bugs and add some missing pieces for some chains

* [FBA-01M, FBA-02M] Native bid claims and reentrancy guard (#455)

* feat: add native claims for outbidding rather than direct native transfer

* chore: add tests for native claims

* chore: add reentrancy guard to bid function

* fix: address PR comments

* chore: add max tax amount to clearCheckoutRequest (#460)

* EYT-01M: Inexistent Requirement of Successful Operation (#461)

* feat: revert on no op tx when no entity has been removed

* refactor: address PR comments

* chore: remove all privileges of old admin role (#462)

* feat: add new proposalId param in voteOnProposal (#463)

* Fix failing unit tests

* Fix failing coverage test

* refactor: remove break in for loops (#479)

* [CNO-01C] Redundant Import of Logic Implementation (#513)

* Introduce IFermionFractionsERC20 interface

* tidy

* tidy

* [ORE-05M] Potential Loss of Native Funds (#512)

* Prevent native funds when seller deposit is 0

* Cover also native self-sale unwrap

* Update test/protocol/offerFacet.ts

Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>

* Update test/protocol/offerFacet.ts

Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>

* Prevent native funds when erc20 offer and sellerDeposit covered from the available funds

* Update test/protocol/offerFacet.ts

Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>

---------

Co-authored-by: 0xlucian <96285542+0xlucian@users.noreply.github.com>

* Audit fixes batch 1.1.0 (#485)

* chore: cvt-01m auction state end change condition

* chore: cyd-02m fix custodian payoff precise return amount revert edge case

* chore: ore-01m remove commented out code

* chore: cyd-01m using _msgSender() in addItemToCustodianOffer external call

* chore: cgi-01m impose paused region restriction on setProtocolFeeTable

* chore: eip-01m remove revert in case of failed call to signer

* chore: eip-02m use block.chainid instead of cached chainId

* chore: fmr-02m manual static call and validation of returnData

* chore: fwr-01s address 0 check

* chore ctn-01s split interface from contract file

* refactor: fix PR comments

* chore: rgd-02c

* chore: ore-03c redundant zero address check

* chore: ore-02c remove redundant assignment

* chore: eyt-03c

* chore: cvt-02c

* refactor: add test for checkTrustedForwarder response handling

* refactor: small fixes

* refactor: add a happy path for safeTrasferFrom if trustedForwarder return strange data

* fix: failing test

* chore: increase code coverage

* set MAX_SKIPPED to 0 and tidy

* Allow multiple price update proposals  (#514)

* feat: initial implementation

* refactor: adjust tests

* chore: ffe-01m adjust votes on burn as well

* refactor: increase coverage and some small code fixes

* refactor: address PR comments

* refactor: small fixes

* chore: add epoch to all FermionFractions events

* Small refactor + tidy

* Fix failing unit tests

---------

Co-authored-by: zajck <klemen@impressieve.com>

* Code Style Batch fixes 2 (#506)

* chore: cyd-01c add unchecked when necessary

* chore: cvt-01c incorect revert natspec

* chore: eyt-02c add unchecked block safe arithmetic

* chore: eyt-04c relocate modifiers to internal function

* chore: flb-01c add safe arithmetich in unchecked block

* chore: fft-01c change emptyslot type to unit256

* chore: ffp-01c add unchecked block to safe arithmetic op

* chore: ffm-01c optimise migrateFractions function

* chore: fsd-01c remove explicit contract invocation

* chore: fmr-02c move safe arithmetics into unchecked block

* fix: ffm-01c optimise migrateFractions function

* refactor: address PR comments

* FIxed unchecked brackets

* Code style fixes batch 1 (#505)

* Fix [ASS-01C] #480

* FIx [CPO-01C] #481

* FIx [CST-01C] #483

* Fix [CTX-01C] #484

* Fix [EYT-01C] #488

* Fix [FFP-02C] #494

* Fix [FMR-01C] #497

* Fix [MTN-01C] #499

* Fix [ORE-01C] #500

* Fix [FFP-02C] #494

* Fix [RGD-01C] #503

* Fix [ORE-02M] Insecure Unchecked Code Block #510

---------

Co-authored-by: Klemen <64400885+zajck@users.noreply.github.com>
Co-authored-by: zajck <klemen@impressieve.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levalleux-ludo levalleux-ludo levalleux-ludo approved these changes

@0xlucian 0xlucian 0xlucian approved these changes

Assignees

@zajck zajck

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zajck @levalleux-ludo @0xlucian