Foundation contest - thankthedark's results

Building the new creative economy

General Information

Platform: Code4rena

Start Date: 24/02/2022

Pot Size: $75,000 USDC

Total HM: 21

Participants: 28

Period: 7 days

Judge: alcueca

Total Solo HM: 15

Id: 94

League: ETH

Foundation

Findings Distribution

Researcher Performance

Rank: 13/28

Findings: 2

Award: $949.97

🌟 Selected for report: 1

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Foundation

Findings Information

🌟 Selected for report: thankthedark

Also found by: Afanasyevich, cmichel

Labels

bug

2 (Med Risk)

sponsor confirmed

Awards

657.3838 USDC - $657.38

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketPrivateSale.sol#L123-L174

Vulnerability details

Impact

Within a NFTMarketPrivateSale contract, buyers are allowed to purchase a seller's NFT. This is done through a seller providing a buyer a EIP-712 signature. The buyer can then call #buyFromPrivateSaleFor providing the v, r, and s values of the signature as well as any additional details to generate the message hash. If the signature is valid, then the NFT is transferred to the buyer.

The problem with the code is that EIP-712 signatures can be re-used within a small range of time assuming that the original seller takes back ownership of the NFT. This is because the NFTMarketPrivateSale#buyFromPrivateSaleFor method has no checks to determine if the EIP-712 signature has been used before.

Proof of Concept

Consider the following example:

Joe the NFT owner sells a NFT to the malicious buyer Rachel via a private sale.
Rachel through this private sale obtains the EIP-712 signature and uses it to purchase a NFT.
Joe the NFT owner purchases back the NFT within two days of the original sale to Rachel.
Joe the NFT owner puts the NFT back on sale.
Rachel, who has the original EIP-712 signature, can re-purchase the NFT by calling #buyFromPrivateSaleFor again with the same parameters they provided in the original private sale purchase in step 1.

The #buyFromPrivateSaleFor function runs several validation checks before transferring the NFT over to the buyer. The validations are as follows:

L#132 - The signature has expired.
L#135 - The deadline is beyond 48 hours from now.
L#143 - The amount argument is greater than msg.value.
L#149 - The msg.value is greater than the amount set.
L#171 - This checks that the EIP-712 signature comes from the NFT seller.

As you can see, there are no checks that the EIP-712 signature has been used before. If the original NFT seller purchases back the NFT, then they are susceptible to having the original buyer taking back the NFT. This can be problematic if the NFT has risen in value, as the original buyer can utilize the same purchase amount from the first transaction in this malicious transaction.

Tools Used

Pen and paper

Recommended Mitigation Steps

Most contracts utilize nonces when generating EIP-712 signatures to ensure that the contract hasn't been used for. When a nonce is injected into a signature, it makes it impossible for re-use, assuming of course the nonce feature is done correctly.

#0 - HardlyDifficult
2022-03-03T12:23:53Z

Yes, this is a good point to raise and something like this could happen because it's not intuitive to think that the private sale remains valid after it's been used.

This is mitigated by the short time window we use for Private Sales. Our frontend uses 24-hour expirations and in the contract we ensure the window is <= 48 hours.

In order to remain backwards compatible with our existing integration and any signatures which are outstanding at the time of the upgrade, we decided not to use nonce as recommended. Instead we simply store a mapping tracking if the exact private sale terms have already been used.

Here's the primary addition:

    // Ensure that the offer can only be accepted once.
    if (privateSaleInvalidated[nftContract][tokenId][msg.sender][seller][amount][deadline]) {
      revert NFTMarketPrivateSale_Signature_Canceled_Or_Already_Claimed();
    }
    privateSaleInvalidated[nftContract][tokenId][msg.sender][seller][amount][deadline] = true;

Findings Information

🌟 Selected for report: Dravee

Also found by: 0x1f8b, CertoraInc, TerrierLover, csanuragjain, gzeon, iain, kenta, rfa, robee, thankthedark

Labels

bug

G (Gas Optimization)

Awards

292.5905 USDC - $292.59

External Links

Link to GitHub issue

Reading array length on each iteration spends more gas instead of caching the array len outside of the loop:

NFTMarketReserveAuction.sol::274 - for (uint256 i = 0; i < listedAuctionIds.length; ++i) {
NFTMarketCreators.sol::94 - for (uint256 i = 0; i < _recipients.length; ++i) {
NFTMarketCreators.sol::155 - for (uint256 i = 0; i < _recipients.length; ++i) {
NFTMarketCreators.sol::193 - for (uint256 i = 0; i < _recipients.length; ++i) {

Use != 0 instead of > 0 for unsigned integer comparisons:

FETH.sol::679 - if (escrow.expiration >= block.timestamp && escrow.totalAmount > 0) {
FETH.sol::699 - if (escrow.expiration >= block.timestamp && escrow.totalAmount > 0) {
NFTMarketFees.sol::196 - if (creatorRecipients.length > 0) {
NFTMarketFees.sol::75 - if (creatorRecipients.length > 1) {
NFTMarketFees.sol::74 - if (creatorFee > 0) {
NFTMarketCreators.sol::90 - if (_recipients.length > 0 && _recipients.length == recipientBasisPoints.length) {
NFTMarketCreators.sol::153 - if (_recipients.length > 0 && _recipients.length == recipientBasisPoints.length) {
NFTMarketCreators.sol::185 - if (_recipients.length > 0) {

The DIV opcode uses more gas in comparison to other viable options, such as the SHR opcode:

FETH.sol::152 - lockupInterval = _lockupDuration / 24;

Initializing variable with the default value is unnecessary and costs additional gas:

NFTMarketOffer.sol::161 - for (uint256 i = 0; i < nftContracts.length; ++i) {
NFTMarketReserveAuction.sol::274 - for (uint256 i = 0; i < listedAuctionIds.length; ++i) {
NFTMarketFees.sol::88 - maxCreatorIndex = 0;
NFTMarketFees.sol::96 - maxCreatorIndex = 0;
NFTMarketCreators.sol::94 - for (uint256 i = 0; i < _recipients.length; ++i) {
NFTMarketCreators.sol::155 - for (uint256 i = 0; i < _recipients.length; ++i) {
NFTMarketCreators.sol::193 - for (uint256 i = 0; i < _recipients.length; ++i) {

#0 - HardlyDifficult
2022-03-03T12:56:42Z

Thanks for the feedback!

Cache len: We have made this update in NFTMarketCreators - it may offer a small gas savings, but it also improves readability since length is accessed multiple times there. We are going to pass on changing the NFTMarketReserveAuction instance at this time though since that's a call reserved for Foundation admins and we are not concerned with optimizations there.
Use != 0: Yes, I find it odd that this makes a difference lol. But another warden suggested the same and we did find a tiny improvement -- so we went ahead with the update.
Div: This is part of the constructor so we are not concerned with optimizing. Additionally we'd like to avoid using inline assembly where possible unless it's a significant savings.
Initializing defaults: We tested this by removing all defaults that were explicitly defined and found absolutely no impact. I think the Solidity optimizer has been improved, which is great because this was a strange practice to encourage.

#1 - alcueca
2022-03-17T16:09:00Z

Score: 500

Foundation contest - thankthedark's results

Building the new creative economy

General Information

Findings Distribution

Researcher Performance

External Links

🌟 [M-01] EIP-712 signatures can be re-used in private sales - $657.38

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - HardlyDifficult2022-03-03T12:23:53Z

[G-04] Gas Report - $292.59

Findings Information

Labels

Awards

External Links

#0 - HardlyDifficult2022-03-03T12:56:42Z

#1 - alcueca2022-03-17T16:09:00Z

#0 - HardlyDifficult
2022-03-03T12:23:53Z

#0 - HardlyDifficult
2022-03-03T12:56:42Z

#1 - alcueca
2022-03-17T16:09:00Z