Delegate Token owner can be griefed during extend()

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L325-L336 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L339-L350

Vulnerability details

Impact

There are 2 truths:-

• First, Principal Token owner can call the extend function in the DelegateToken contract . extend allows to increase the expiry timestamp of the particular Delegate Token Id.
• Second, there is another function called rescind, it allows anyone to forcefully return the DelegateToken Id back to the PT owner if it has expired.

Now, if we combine the effects of these two functions, they have the following impact.

Example:- PT owner has delegated a BAYC NFT. The delegated owner of the NFT privately requests the PT owner to extend the delegation rights in anticipation of an airdrop. PT owner privately takes $5000 from DT owner as their share in speculation of an airdrop. PT owner calls extend as per the deal. But privately they front run their own call by calling rescind through an Anonymous address, this transfers the DT token back to the principal Token owner.

End result is:- PT owner now owns DT as well. There's no way to prove that the PT owner was behind the attack.

Proof of Concept

Paste this test in DelegateToken.t.sol and run forge test --mt testPTRugsDT

   function testPTRugsDT(address tokenOwner, address dtTo, address notLdTo, address principalTo, uint256 amount, bool expiryTypeRelative) public {
        uint time = block.timestamp + 100;
        vm.assume(tokenOwner != address(0));
        vm.assume(principalTo != address(0));
        vm.assume(notLdTo != dtTo);
        vm.assume(tokenOwner != address(dt));
        vm.assume(dtTo != address(0));
        vm.assume(amount != 0);

        ( /* ExpiryType */ , uint256 expiry, /* expiryValue */ ) = prepareValidExpiry(expiryTypeRelative, time);

        mockERC20.mint(tokenOwner, amount);
        vm.startPrank(tokenOwner);
        mockERC20.approve(address(dt), amount);
        uint256 delegateId =
        dt.create(DelegateTokenStructs.DelegateInfo(principalTo, IDelegateRegistry.DelegationType.ERC20, dtTo, amount, address(mockERC20), 0, "", expiry), SALT);
        vm.stopPrank();
    // Current DT owner is dtTo address
        assert(dt.ownerOf(delegateId) == dtTo);
        vm.prank(makeAddr("PRINCIPAL_OWNER_SECRET_WALLET"));
        vm.warp(expiry + 1);
        dt.rescind(delegateId);// Transfers the DT back to PT owner.
        assert(principal.ownerOf(delegateId) == principalTo);
        assert(dt.ownerOf(delegateId) == principalTo);
        vm.prank(principalTo);
        // @audit No use of calling extend now as DT is already transferred to PT.
        dt.extend(delegateId,block.timestamp + 200);

    }

Tools Used

Manual

Recommended Mitigation Steps

Create another function(say FinalRescind) that should need to be called after a block gap after the rescind call which will actually transfer the DT. We will again check for expiry conditions in FinalRescind also so that the extend call has the appropriate effect.

Assessed type

DoS

[0xfoobar]

Rescinding is permissionless after DT expiration. Extending is optional for the PT owner. Anonymous stealth addresses do not matter here, the PT owner can simply not extend for identical behavior. Not a bug, intended behavior.

[c4-sponsor]

0xfoobar (sponsor) disputed

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Possible griefing but if a person owns both tokens they can always claim their underlying

[c4-judge]

GalloDaSballo marked the issue as grade-c