The `DelegateToken.approve` function will directly overwrite the old user's approval, resulting in a loss of user rights

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

DelegateToken.approve will override the old approval with the same delegateId. This will directly impact the rights of users who were previously approved, effectively revoking their previous approval.

    function approve(address spender, uint256 delegateTokenId) external {
        bytes32 registryHash = StorageHelpers.readRegistryHash(delegateTokenInfo, delegateTokenId);
        StorageHelpers.revertNotMinted(registryHash, delegateTokenId);
        address delegateTokenHolder = RegistryHelpers.loadTokenHolder(delegateRegistry, registryHash);
        StorageHelpers.revertNotOperator(accountOperator, delegateTokenHolder);
        //@audit override the approval
        StorageHelpers.writeApproved(delegateTokenInfo, delegateTokenId, spender);
        emit Approval(delegateTokenHolder, spender, delegateTokenId);
    }

    function writeApproved(mapping(uint256 delegateTokenId => uint256[3] info) storage delegateTokenInfo, uint256 delegateTokenId, address approved) internal {
        uint96 expiry = uint96(delegateTokenInfo[delegateTokenId][PACKED_INFO_POSITION]);
        delegateTokenInfo[delegateTokenId][PACKED_INFO_POSITION] = (uint256(uint160(approved)) << 96) | expiry;
    }

Proof of Concept

There three users: from, to and charle. from creates a delegateId and approves it to to, then approves it to charle. Now, to and charle should be able to operate on this delegateId. However, to's authorization has been overridden by charle and to can no longer transfer this delegateId.

Add the test to test/DelegateToken.t.sol, then run it with forge test --match-test testApprove -vvv.

diff --git a/test/DelegateToken.t.sol b/test/DelegateToken.t.sol
index 1040d7d..92d155f 100644
--- a/test/DelegateToken.t.sol
+++ b/test/DelegateToken.t.sol
@@ -195,6 +195,29 @@ contract DelegateTokenTest is Test, BaseLiquidDelegateTest {

         assertFalse(registry.checkDelegateForERC721(from, address(dt), address(mockERC721), underlyingTokenId, ""));
     }
+    function testApprove() public {
+        address to = address(1);
+        address from = address(2);
+        address charle = address(3);
+        uint256 underlyingAmount = 100;
+        bool expiryTypeRelative = false;
+        uint256 time = 100;
+
+        ( /* ExpiryType */ , uint256 expiry, /* ExpiryValue */ ) = prepareValidExpiry(expiryTypeRelative, time);
+        mockERC20.mint(address(from), underlyingAmount);
+
+        vm.startPrank(from);
+        mockERC20.approve(address(dt), underlyingAmount);
+        uint256 delegateId =
+            dt.create(DelegateTokenStructs.DelegateInfo(from, IDelegateRegistry.DelegationType.ERC20, from, underlyingAmount, address(mockERC20), 0, "", expiry), SALT);
+        dt.approve(to, delegateId);
+        dt.approve(charle, delegateId);
+        vm.stopPrank();
+
+        vm.prank(to);
+        dt.transferFrom(from, to, delegateId);
+
+    }

     function testFuzzingWithdraw20Immediately(address from, uint256 underlyingAmount, bool expiryTypeRelative, uint256 time) public {
         vm.assume(from != address(0));

Result:

Failing tests:
Encountered 1 failing test in test/DelegateToken.t.sol:DelegateTokenTest
[FAIL. Reason: NotApproved(0x0000000000000000000000000000000000000001, 83125656321449047896925968989594772657464967156116746192883976214766562556269 [8.312e76])] testApprove() (gas: 453013)

Tools Used

forge

Recommended Mitigation Steps

Maybe the approval for a single delegateId should also be stored in a mapping like accountOperator, rather than stored in the DelegateTokenStorageHelpers.

Assessed type

Access Control

[code423n4]

Withdrawn by Krace