c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

A malicious PrincipalToken owner can call the extend() function through a front-running attack before transferring it to other users, causing the period to be extended, thereby locking other users' funds.

According to the documentation:

If the user owns both delegate token and principal token, they can redeem it directly before expiration.
If you only have the principal token, you can only redeem it after it expires.

Let's imagine the following scenario:

Alice thinks that the fund pool to which Bob's principal token belongs is profitable, and then purchases it from Bob. Through the transaction between the two, Alice got the principal token, which she only needs to wait until the expiration time to redeem it. At this time, Bob is a malicious user. He executes the extend() function through a front-running attack and extends the time. As a result, the principal token obtained by Alice cannot be redeemed. And because Alice does not own the delegate token, she cannot redeem it before expiration. This will cause Alice to funds are locked

Proof of Concept

As we can see, extend() function can be called by the principalToken owner or approver

    function extend(uint256 delegateTokenId, uint256 newExpiry) external {
        StorageHelpers.revertNotMinted(delegateTokenInfo, delegateTokenId);
        Helpers.revertOldExpiry(newExpiry);
        uint256 previousExpiry = StorageHelpers.readExpiry(delegateTokenInfo, delegateTokenId);
        if (newExpiry <= previousExpiry) revert Errors.ExpiryTooSmall();
        if (PrincipalToken(principalToken).isApprovedOrOwner(msg.sender, delegateTokenId)) {
            StorageHelpers.writeExpiry(delegateTokenInfo, delegateTokenId, newExpiry);
            emit ExpiryExtended(delegateTokenId, previousExpiry, newExpiry);
            return;
        }
        revert Errors.NotApproved(msg.sender, delegateTokenId);
    }

and the withdraw() will call StorageHelpers.revertInvalidWithdrawalConditions(delegateTokenInfo, accountOperator, delegateTokenId, delegateTokenHolder); to check if expiry. Last , it will burn the Principal token by calling StorageHelpers.burnPrincipal()

So:

If the user owns both delegate token and principal token, they can redeem it directly before expiration.
If you only have the principal token, you can only redeem it after it expires.

Tools Used

manual

Recommended Mitigation Steps

I think this problem is similar to a slippage attack. We can add a parameter like real_expiration_time to PrincipalToken.transfer() function. This parameter is specified by the user. When calling PrincipalToken.transfer() function , call DelegateToken.getDelegateInfo() to check whether it is the correct real_expiration_time.