c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/PrincipalToken.sol#L35

Vulnerability details

Vulnerability Details

_mint function does not check whether the receiver accepts the ERC721 tokens or not such as smart contracts.

On the other hand, the _safeMint function call the onERC721Received hook on the receiver end (if it is the contract) which validates that the contract accept the ERC721 tokens.

Impact

The NFT will be permanatly locked

Tools Used

Manual Analysis

Recommended Mitigation Steps

/// @notice Mints a PT if and only if the DT contract calls and has authorized
function mint(address to, uint256 id) external {
    _checkDelegateTokenCaller();

-    _mint(to, id);
+    _safeMint(to, id);

    IDelegateToken(delegateToken).mintAuthorizedCallback();
}

Use _safeMint instead of _mint function.

Assessed type

ERC721

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

alymurtazamemon commented 1 year ago

Hello Sir,

I respect your evaluation here and I am also a beginner so not in a state to question seniors' judgement.

But I want to provide some information here due to that I think my selection of the medium issue was right.

Below is the text from the contest description and here we can see that the PrincipalToken provides the holder with the right to reclaim their deposited NFT from escrow once the escrow period expires..

The delegate marketplace consists of three core contracts: the DelegateToken, the PrincipalToken, and the CreateOfferer. Users will deposit a token, such as a bored ape NFT, into smart contract escrow using the DelegateToken.sol::create() function. They will receive back two ERC721s: a bored ape DelegateToken, and a bored ape PrincipalToken. The holder of the DelegateToken will receive delegate rights for the duration of the escrow. The holder of the PrincipalToken will have the right to redeem the bored ape from escrow at conclusion of the chosen timeframe. Users can choose to transfer or sell neither, one, or both of these. The CreateOfferer is a Seaport Contract Offerer that enables gasless listing of DelegateTokens which have not been created yet. If a buyer fulfills the gasless listing, then the desired token will be atomically escrowed and a DelegateToken created.

From this, we can conclude that if users lose their PrincipalToken then they will also lose their deposited NFT.

This can happen from the PrincipalToke.sol contract side where it uses the _mint function which does not check whether the receiver is compatible enough to hold the ERC721 token or not, and mint the token for that address (as explained above).

OWASP Severity Matrix

Likelihood(Row) * Impact(Column) = Severity

	High	Medium	Low
High	Critical	High	Medium
Medium	High	Medium	Low
Low	Medium	Low	Note

If we put our issue into this matrix and calculate the Severity level then it will be between Medium and High. Because the loss of an asset is always considered as a High Impact and if we let's say consider the Likelihood low then still it is in the Medium Severity category.

Supporting References

There are countless examples available on Solodit.

Thanks

GalloDaSballo commented 1 year ago

The issue has long been downgraded to QA as self-rekt If you buy a token and send it to a recipient that cannot use it, it's your mistake

See extensive discussion here: https://github.com/code-423n4/org/issues/53

code-423n4 / 2023-09-delegate-findings

Use `_safeMint` function instead of `_mint` function to NFTs from being locked #310