Principal token can be permanently locked

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L321 https://github.com/code-423n4/2023-09-delegate/blob/main/src/PrincipalToken.sol#L33-L37

Vulnerability details

Impact

When a user locks their tokens inside the escrow, they can choose where to receive the principal token, which is necessary to withdraw their original tokens.

In some scenarios, this could result in a locked principal if the receiver is a smart contract that expects a onERC721Received callback.

Proof of Concept

1. Bob calls DelegateToken.create, and he provides a smart contract address as a delegateInfo.principalHolder which expects an onERC721Received callback.

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

2. mintPrincipal is called:

function mintPrincipal(address principalToken, Structs.Uint256 storage principalMintAuthorization, address principalRecipient, uint256 delegateTokenId) internal {
    if (principalMintAuthorization.flag == MINT_NOT_AUTHORIZED) {
        principalMintAuthorization.flag = MINT_AUTHORIZED;
        PrincipalToken(principalToken).mint(principalRecipient, delegateTokenId);
        principalMintAuthorization.flag = MINT_NOT_AUTHORIZED;
        return;
    }
    revert Errors.MintAuthorized();
}

which calls PrincipalToken(principalToken).mint:

function mint(address to, uint256 id) external {
    _checkDelegateTokenCaller();
    _mint(to, id);
    IDelegateToken(delegateToken).mintAuthorizedCallback();
}

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

3. _mint will not trigger an onERC721Received callback, so the principal will be sent and locked inside the contract.
4. As the principal is necessary to withdraw the original token, Bob has his funds permanently locked.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using safeMint instead of mint when minting a PrincipalToken:

    function mint(address to, uint256 id) external {
        _checkDelegateTokenCaller();
-       _mint(to, id);
+       _safeMint(to, id);
        IDelegateToken(delegateToken).mintAuthorizedCallback();
    }

Assessed type

ERC721

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[DadeKuma]

I believe this is a valid medium, as it's not self-inflicted.

Please note that, unlike other similar duplicates that state this will occur if the user targets a contract that does not have an onERC721Received hook (i.e. user error), I'm stating that this issue can occur even if the user does everything correctly (i.e., when the user transfers to a delegateInfo.principalHolder that has an onERC721Received hook):

In some scenarios, this could result in a locked principal if the receiver is a smart contract that expects an onERC721Received callback.

This is very likely to occur, especially when delegateInfo.principalHolder != msg.sender and delegateInfo.principalHolder is not an EOA.

[GalloDaSballo]

If you can buy X And you send it to Y And Y requires you to call it via safeTransfer And you don't You made a mistake

That's why it's self-rekt

If Alice wants to protect the token by adding it to a magical contract that needs it to receive a callback ontransfer, she can mint and then transfer the token

The system has to defend itself, not some other contract