DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Wrong status set for new created short record in `transferShortRecord`

CipherWizard

Summary

When the transferShortRecord function is invoked during the transfer of a short record, it generates a new short record for the recipient. However, it consistently assigns the short status as SR.FullyFilled, regardless of the previous status. This can result in an erroneous classification of a partially filled short record as fully filled.

Vulnerability Details

The issue occurs in the transferShortRecord function :

function transferShortRecord(
address asset,
address from,
address to,
uint40 tokenId,
STypes.NFT memory nft
) internal {
AppStorage storage s = appStorage();
STypes.ShortRecord storage short = s.shortRecords[asset][from][
nft.shortRecordId
];
if (short.status == SR.Cancelled)
revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
​
deleteShortRecord(asset, from, nft.shortRecordId);
​
uint8 id = createShortRecord(
asset,
to,
SR.FullyFilled, //@audit always sets status to FullyFilled
short.collateral,
short.ercDebt,
short.ercDebtRate,
short.zethYieldRate,
tokenId
);
​
if (id == Constants.SHORT_MAX_ID) {
revert Errors.ReceiverExceededShortRecordLimit();
}
​
s.nftMapping[tokenId].owner = to;
s.nftMapping[tokenId].shortRecordId = id;
}

As observed, the function utilizes createShortRecord to generate a new record for the recipient's address, consistently setting the status as SR.FullyFilled. This approach is flawed, as the short record may potentially be in a partially filled state at the time of transfer. Assigning it the status of fully filled immediately renders it impossible to refill in the future.

Consequently, this could result in a loss of funds for the new owner of the short record, as they would be unable to match the short again.

It's important to note that neither ERC721Facet.mintNFT nor ERC721Facet.transferFrom prohibit the use of partially filled shorts. Therefore, it is conceivable that some individuals transferring their short records may encounter this issue.

Impact

Incorrectly designating a short record as fully filled during its transfer could lead to potential financial losses for the protocol or the individual short seller, as this short position would then become ineligible for future filling.

Tools Used

Manual review

Recommended Mitigation

Ensure that the newly created short record is assigned the appropriate status when performing a transfer :

function transferShortRecord(
address asset,
address from,
address to,
uint40 tokenId,
STypes.NFT memory nft
) internal {
AppStorage storage s = appStorage();
STypes.ShortRecord storage short = s.shortRecords[asset][from][
nft.shortRecordId
];
if (short.status == SR.Cancelled)
revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
​
deleteShortRecord(asset, from, nft.shortRecordId);
​
uint8 id = createShortRecord(
asset,
to,
short.status, //@audit use current short status
short.collateral,
short.ercDebt,
short.ercDebtRate,
short.zethYieldRate,
tokenId
);
​
if (id == Constants.SHORT_MAX_ID) {
revert Errors.ReceiverExceededShortRecordLimit();
}
​
s.nftMapping[tokenId].owner = to;
s.nftMapping[tokenId].shortRecordId = id;
}
Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.