Malicious users may grief the expiry date right before PrincipalToken is sold on the secondary market

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

With the PrincipalToken token owners can withdraw assets from the DelegateToken contract:

• after the delegation has expired
• when the delegate token owner rescinds.

Additionally, the DelegateToken contract has a function extend where principal token owners may increase the expiration date (max date is type(uint96).max).

Furthermore, in the overview of the audit details, Delegate team has specified

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.
...
Users can choose to transfer or sell neither, one, or both of these.

This means that the DelegateToken token and PrincipalToken token are expected to be sold on the secondary market.

This allows a malicious user to list his principal token on the secondary market and set the expiry date to type(uint96).max right before the token is sold, which would essentially lock/burn the assets forever.

Proof of Concept

As mentioned before, there are 2 ways to withdraw assets from DelegateToken as principal token owner:

1. Wait for the delegation to expire.
2. The delegate token owner rescinds, which allows the principal token owner to withdraw.

Here is the DelegateToken.extend function that allows principal token owners to increase the expiry date:

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

Example

As an example, I will take Opensea as the ERC721 secondary market. The way Opensea works is that users sign permission for Opensea to transfer their ERC721 tokens when the sale happens, while those tokens sit in users' wallets.

Bob the attacker owns 2 Bored Apes at a 28 ETH valuation.

Bob delegates 1 Bored Ape in DelegateToken with the following rules:

• Delegated to himself
• Expires in 5 days

Bob's next actions are as follows:

1. Bob lists his Principal token on Opensea for 27 ETH
2. Alice the victim sees that she can buy the rights to a Bored Ape that is going to unlock in 5 days and the price is cheaper than the market price. She buys it and the transaction is in the mempool.
3. Bob sees that Alice is buying his principal token and calls DelegateToken.extend and sets the expiry date to type(uint96).max, which is year 4480 which essentially means that the token is locked forever/burned.
4. After 5 days Alice tries to withdraw the Bored Ape, but she can't. Then she realizes that the expiry date has been set to type(uint96).max.

In conclusion - Alice loses 27 ETH while Bob has reduced the Bored Ape total supply and his other Bored Ape has risen in value because of it.

Tools Used

Manual Review

Recommended Mitigation Steps

The DelegateToken.extend function is too risky for buyers on the secondary market.

My suggestion is to remove the DelegateToken.extend function completely. If there are users that want to increase the expiry date, my suggestion is:

1. Wait for the delegation to expire
2. Withdraw the delegated asset
3. Create a delegation for it once again with a higher expiry date.

Assessed type

Context

[GalloDaSballo]

Flagging but I think if you hold both tokens you can rescind

[c4-sponsor]

0xfoobar (sponsor) acknowledged

[0xfoobar]

Extend-frontrunning is a valid griefing issue, though unclear why people would do this (and PT holders can rescind if they get ahold of the DT).

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

This is a valid concern to check, but ultimately the burden is on the purchaser + they have the ability to withdraw at any time if they also own the Delegate Token

So arguably an OTC Buyer should:

• Purchase Delegate token first
• Then buy Principal Token

I think it's best categorized as QA / Gotcha