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

1 stars 0 forks source link

QA Report #67

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Most contracts in the code base use a fixed solidity compiler version 0.8.13. However, the TokenTransferrer contract, and the few Constants/Enums/Structus contracts use solidity compiler version >=0.8.7. There could potentially be breaking changes when Solidity upgrades to 0.9.0 and above, and using unlocked compiler version >=0.8.7 is unsafe and could potentially create unintended consequences. Recommend using 0.8.13 throughout the code base instead.

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

0age commented 2 years ago

needed for reference contracts that use the same contracts

0xleastwood commented 2 years ago

As per the sponsor, this contract is needed.

GalloDaSballo commented 2 years ago

Invalid because:

IllIllI000 commented 2 years ago

@GalloDaSballo If the pragma were using '^' rather than '>=' I'd agree that there is no issue, but '>=' will not cut off at version 0.9.0 like '^' does. The recommendation may be wrong, but the version check will have an issue once there are breaking changes. Others importing the interface may not be using the same foundry.toml

GalloDaSballo commented 2 years ago

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

Code will still always compile down to specified version: https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/foundry.toml#L27

IllIllI000 commented 2 years ago

and the few Constants/Enums/Structus contracts

e.g. if I am coding against seaport contracts in my own 0.9.0 project and am importing this: https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/ConsiderationEnums.sol#L2-L17 and solidity version 0.9.0 changes the values of the enums, my code will be broken when submitting transactions to the 0.8.13 version in seaport's foundry.toml

GalloDaSballo commented 2 years ago

From cross-comparison of a bunch of contests judged by me, HardlyDifficult and Leastwood, Floating Pragma is not a vulnerability, given that the sponsor did implement and that historically the finding was marked as valid, will rate NC and re-valuate the submissions

GalloDaSballo commented 2 years ago

1NC