search

code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

Basic order fulfillment can get into a re-enterable state #114

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/BasicOrderFulfiller.sol#L1073 https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/BasicOrderFulfiller.sol#L291

Vulnerability details

Impact

The basic order fulfillment function is re-enterable.

Proof of Concept

Reentrancy can happen in the call to _triggerIfArmed in the _validateAndFulfillBasicOrder function of the BasicOrderFulfiller. That's because the _clearReentrancyGuard function is called in the end of the _transfer{Eth, ERC20}AndFinalize function and there can be accumulated tokens that hasn't been transferred yet, which will be transferred after the reentrancy guard was cleared.

Tools Used

VS Code, Remix and my brain.

Recommended Mitigation Steps

Call the _clearReentrancyGuard only at the end of the _validateAndFulfillBasicOrder function. An easy replacement to these calls is to use a modifier like the open-zeppelin reentrancy guard implementation suggest, which takes care of the reentrancy guard values.

0age commented 2 years ago

This is a valid finding, and we intend to fix it to remain consistent; but it also occurs as the final step of order fulfillment, after all checks and effects have taken place, and so the implications are not nearly as severe.

HardlyDifficult commented 2 years ago

This identifies a missing reentrancy guard but not a vulnerability that could arise as a result, so lowering this to a low severity. Grouping with the warden's QA report #110