withdraw() function is vulnerable to permanently bricking tokens if it fails after setting the registry hash to the "used" flag.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L354-L357

Vulnerability details

Impact

If withdraw() fails later (e.g. due to lack of funds when transferring tokens), the registry hash has already been marked "used" and can never be changed again. This permanently bricks the token.

Proof of Concept

The key issue is that writeRegistryHash() is called first, before any checks on the validity of the withdrawal. This sets the registry hash to the "used" flag.

If any of the subsequent checks fail, such as revertNotMinted(), the function will revert. However, the registry hash has already been set to "used". This means the token is now permanently marked as withdrawn, even though the actual withdrawal did not occur. A proof of concept exploit would be:

1. Call withdraw()
2. Let it set registry hash to "used"
3. Force it to fail later by making the token transfer fail (e.g. by setting a low gas limit)
4. Token is now bricked forever

So in summary:

• writeRegistryHash() prematurely sets token to "withdrawn" state
• If later checks fail, withdraw() reverts
• But token remains in "withdrawn" state, meaning it is permanently bricked

Tools Used

Manual

Recommended Mitigation Steps

Only set the "used" flag after all the withdrawal logic has completed successfully. writeRegistryHash() should be moved to the end, after all validity checks pass

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid