c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Proof of Concept

The DelegateToken.withdraw is used for the Principal Token Holder to burn his Principal Token and withdraw the underlying asset:

function withdraw(uint256 delegateTokenId) external nonReentrant {
    bytes32 registryHash = StorageHelpers.readRegistryHash(delegateTokenInfo, delegateTokenId);
    StorageHelpers.writeRegistryHash(delegateTokenInfo, delegateTokenId, bytes32(StorageHelpers.ID_USED));
    // Sets registry pointer to used flag
    StorageHelpers.revertNotMinted(registryHash, delegateTokenId);
    (address delegateTokenHolder, address underlyingContract) = RegistryHelpers.loadTokenHolderAndContract(delegateRegistry, registryHash);
    StorageHelpers.revertInvalidWithdrawalConditions(delegateTokenInfo, accountOperator, delegateTokenId, delegateTokenHolder);
    StorageHelpers.decrementBalance(balances, delegateTokenHolder);
    delete delegateTokenInfo[delegateTokenId][StorageHelpers.PACKED_INFO_POSITION]; // Deletes both expiry and approved
    emit Transfer(delegateTokenHolder, address(0), delegateTokenId);
    IDelegateRegistry.DelegationType delegationType = RegistryHashes.decodeType(registryHash);
    bytes32 underlyingRights = RegistryHelpers.loadRights(delegateRegistry, registryHash);
    if (delegationType == IDelegateRegistry.DelegationType.ERC721) {
        uint256 erc721UnderlyingTokenId = RegistryHelpers.loadTokenId(delegateRegistry, registryHash);
        RegistryHelpers.revokeERC721(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc721UnderlyingTokenId, underlyingRights);
        StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
        IERC721(underlyingContract).transferFrom(address(this), msg.sender, erc721UnderlyingTokenId);
    } else if (delegationType == IDelegateRegistry.DelegationType.ERC20) {
        uint256 erc20UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
        StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
        RegistryHelpers.decrementERC20(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc20UnderlyingAmount, underlyingRights);
        StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
        SafeERC20.safeTransfer(IERC20(underlyingContract), msg.sender, erc20UnderlyingAmount);
    } else if (delegationType == IDelegateRegistry.DelegationType.ERC1155) {
        uint256 erc1155UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
        StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
        uint256 erc11551UnderlyingTokenId = RegistryHelpers.loadTokenId(delegateRegistry, registryHash);
        RegistryHelpers.decrementERC1155(
            delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc11551UnderlyingTokenId, erc1155UnderlyingAmount, underlyingRights
        );
        StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
        IERC1155(underlyingContract).safeTransferFrom(address(this), msg.sender, erc11551UnderlyingTokenId, erc1155UnderlyingAmount, "");
    }
}

In the code above, it's visible that there are two access controls:

First is revertInvalidWithdrawalConditions, which checks some withdraw conditions. If the expiry date has passed, then It doesn't revert. Then, the condition checks are verified using (OR) operator. So, if delegateTokenHolder == msg.sender, this function will not revert, which allows the Delegate Token Holder to call it.

function revertInvalidWithdrawalConditions(
    mapping(uint256 delegateTokenId => uint256[3] info) storage delegateTokenInfo,
    mapping(address account => mapping(address operator => bool enabled)) storage accountOperator,
    uint256 delegateTokenId,
    address delegateTokenHolder
) internal view {
    //slither-disable-next-line timestamp
    if (block.timestamp < readExpiry(delegateTokenInfo, delegateTokenId)) {
        if (delegateTokenHolder == msg.sender || msg.sender == readApproved(delegateTokenInfo, delegateTokenId) || accountOperator[delegateTokenHolder][msg.sender]) {
            return;
        }
        revert Errors.WithdrawNotAvailable(delegateTokenId, readExpiry(delegateTokenInfo, delegateTokenId), block.timestamp);
    }
}

The next access control is RegistryHelpers.revokeERC721/20/1155 and it basically checks if a delegation exists, which is true since we are withdrawing with an active Delegation Token.

function revokeERC721(
    address delegateRegistry,
    bytes32 registryHash,
    address delegateTokenHolder,
    address underlyingContract,
    uint256 underlyingTokenId,
    bytes32 underlyingRights
) internal {
    if (IDelegateRegistry(delegateRegistry).delegateERC721(delegateTokenHolder, underlyingContract, underlyingTokenId, underlyingRights, false) == registryHash) {
        return;
    }
    revert Errors.HashMismatch();
}

Then, after Delegation Token Holder passes all these access controls, a ERC721/1155/20.transferFrom is used using msg.sender as to.
```
        IERC721(underlyingContract).transferFrom(address(this), msg.sender, erc721UnderlyingTokenId);
```
The Delegate Token Holder gets the Underlying Asset without owning the Principal Token.

Impact

If rescind is not used after Expiry Date, delegation Token Holder can steal the Underlying Asset without having the Principal Token or permission for that.

Tools Used

Manual Review

Recommended Mitigation Steps

Check if msg.sender is the owner of Principal Token

require(IERC721(principalToken).ownerOf(delegateTokenId) == msg.sender, "Not the owner of the Principal Token");

Or use IERC721(principalToken).ownerOf(delegateTokenId) as the target of ERC20/1155/721.transferFrom.

Assessed type

Access Control

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

c4-sponsor commented 1 year ago

0xfoobar (sponsor) confirmed

GalloDaSballo commented 1 year ago

The POC invalidates the finding

function testFuzzedERC721Withdraw(address from, uint256 underlyingTokenId) public {
        address from = address(0xadadadadada);
        uint256 underlyingTokenId = 98765432;
        address delegationRecipient = address(0x4a5df);

        bool expiryTypeRelative = true;
        uint256 time = 123;

        ( /* ExpiryType */ , uint256 expiry, /* ExpiryValue */ ) = prepareValidExpiry(expiryTypeRelative, time);
        mockERC721.mint(address(from), underlyingTokenId);

        vm.startPrank(from);
        mockERC721.setApprovalForAll(address(dt), true);
        uint256 delegateId =
            dt.create(DelegateTokenStructs.DelegateInfo(from, IDelegateRegistry.DelegationType.ERC721, delegationRecipient, 0, address(mockERC721), underlyingTokenId, "", expiry), SALT);

        address principalOwner = principal.ownerOf(delegateId);
        console2.log("principalOwner", principalOwner);
        address dtRecipient = dt.ownerOf(delegateId);
        console2.log("dtRecipient", dtRecipient);

        // Wait for expiry
        vm.warp(expiry + 1); // More than expiry

        // Verify Expiry
        console2.log("expiry", (dt.getDelegateInfo(delegateId)).expiry); 
        console2.log("now", block.timestamp);

        // Call withdraw (without PT)
        vm.stopPrank();
        vm.prank(delegationRecipient);
        dt.withdraw(delegateId); /// @audit Reverts here

        address ownerOfUnderlying = mockERC721.ownerOf(underlyingTokenId);
        console2.log("ownerOfUnderlying", ownerOfUnderlying);

        // Verification
        assertFalse(registry.checkDelegateForERC721(from, address(dt), address(mockERC721), underlyingTokenId, ""));

        assertEq(ownerOfUnderlying, from, "Stolen");
    }

The principalToken is burned from the caller

Meaning only they can call withdraw and receive the token

@0xfoobar lmk if I missed anything

0xfoobar commented 1 year ago

There's authentication in StorageHelpers.burnPrincipal() which requires that msg.sender be the owner or approved operator of the PT. So no risk here, invalid issue: https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenStorageHelpers.sol#L72

c4-sponsor commented 1 year ago

0xfoobar (sponsor) disputed

GalloDaSballo commented 1 year ago

Per the above, closing as invalid