search

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

1 stars 0 forks source link

Underflow unchecked #96

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/AmountDeriver.sol#L49-L51

Vulnerability details

Impact

There is no check for underflow even though there is chance to be on

Proof of Concept

in The call for function _locateCurrentAmount( they said that duration !=0.

But, there is this call https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderCombiner.sol#L275

where there duration = advancedOrder.parameters.endTime - advancedOrder.parameters.startTime, where advancedOrder = advancedOrders[I]; (https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderCombiner.sol#L183)

this function (_validateOrdersAndPrepareToFulfill()) called here https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderCombiner.sol#L119

and the function _fulfillAvailableAdvancedOrders() called for example here https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Consideration.sol#L215

which means that the caller set the parameters and can cause an Underflow!

and the function _fulfillAvailableAdvancedOrders() called also in: https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Consideration.sol#L315 and the caller create the args!!

Recommended Mitigation Steps

remove the unchecked

0age commented 2 years ago

_validateOrderAndUpdateStatus ensures that startTime < endTime and so duration must be non-zero; if this is indeed reported by Certora then we'll want to follow up with them to confirm that this was found using manual review (and that they missed the above check) and not via automated tooling or formal verification.

HardlyDifficult commented 2 years ago

Before the call mentioned in the POC, the order is validated which will _verifyTime -- in that function it's indirectly confirmed that startTime < endTime. Therefore the original assumption that duration !=0 holds true either via a revert or by returning (0,0,0) after _validateOrderAndUpdateStatus

dabushori commented 2 years ago

@0age The CertoraInc handle is a team that its members are working in Certora, but we don't use any automated tool or formal verification to address findings, we only manually review the code (the issues reported by CertoraInc in this contest are not mine, but this is true for all the members of the team).

Just making things clear, although the finding is invalid.