use SafeMint instead of mint in PrincipalToken.sol

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The principal token contract mints an nft to a user when a Delegate.create() is called.

function create(Structs.DelegateInfo calldata delegateInfo, uint256 salt) external nonReentrant returns (uint256 delegateTokenId) {
        TransferHelpers.checkAndPullByType(erc1155PullAuthorization, delegateInfo);
        Helpers.revertOldExpiry(delegateInfo.expiry);
        if (delegateInfo.delegateHolder == address(0)) revert Errors.ToIsZero();
.
.
.
    }
        StorageHelpers.mintPrincipal(principalToken, principalMintAuthorization, delegateInfo.principalHolder, delegateTokenId);
    }

From the StorageHelpers.mintPrincipal, we see that the mint function is called passing in the address without checks. This address could be a contract which is unequipped to handle the token being minted to it;

    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();
    }

Proof of Concept

    //@audit-issue use safemint, not _mint
    function mint(address to, uint256 id) external {
        _checkDelegateTokenCaller();
        _mint(to, id);
        IDelegateToken(delegateToken).mintAuthorizedCallback();
    }

As seen above _mint() is used instead of _safemint() which would ensure the tokens are not lost forever in a contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Use _safemint() instead of _mint() to mint the nft to the user

Assessed type

ERC721

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[0xfoobar]

Wholeheartedly disagree, safeMint is unsafe and introduces undesirable novel attack vectors. Onus is on the user to specify correct receiver addresses

[c4-sponsor]

0xfoobar (sponsor) disputed

[GalloDaSballo]

Siding with the Sponsor on this one

[c4-judge]

GalloDaSballo marked the issue as grade-c

[Josephdara]

Hi @GalloDaSballo This is not an invalid, rather it should be a valid medium, Some users can implement transfers function in the onERC721Recieved() of their contracts, however this never gets triggered and the Principal Token is lost. SafeMint cannot trigger any unsafe novel attacks in this case by examining the function the mint is the last call done in create after all checks and updates has been done, https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L320-L322

[0xfoobar]

No, it is not a medium, it is invalid. We explicitly designed to not use safeMint. safeMint is more unsafe and more gas-expensive.