c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

The withdraw function in the DelegateToken contract does not check the caller msg.sender when the expiration time has ended, thus anyone can make a call to withdraw to steal the funds/tokens associated with a delegate token owned by another user after the expiration when those tokens were supposed to be withdrawn only by the PrincipalToken holder.

Proof of Concept

The issue occurs in the DelegateToken.withdraw function below :

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
        );

    // @audit checking for caller and expiration time
    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
        );

        // @audit token transferred to msg.sender
        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
        );

        // @audit token transferred to msg.sender
        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
        );

        // @audit token transferred to msg.sender
        IERC1155(underlyingContract).safeTransferFrom(
            address(this),
            msg.sender,
            erc11551UnderlyingTokenId,
            erc1155UnderlyingAmount,
            ""
        );
    }
}

As it can be seen from the code above, before burning the delegate token and sending funds to the caller the withdraw function calls revertInvalidWithdrawalConditions which is responsible of checking the expiration time and if the caller is allowed to call the withdraw function.

/// @dev should only revert if expiry has not expired AND caller is not the delegateTokenHolder AND not approved for the delegateTokenId AND not an operator for
/// delegateTokenHolder
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
        );
    }

    // @audit Does not check caller if expiration time has passed
    // @audit So anyone can call the `withdraw` function
}

From the code above we can notice that the function checks the caller address msg.sender only when the expiration time has not yet ended, thus when the expiration time has passed this check won't revert when the function is called by a random user and so any user will be able to withdraw any delegate token in that case.

Because when the expiration time is ended any user will be able to call the withdraw function, this issue will lead to the loss of the escrowed tokens (it doesn't matter which type) as the withdraw function will always transfer the tokens (ERC721, ERC20, ERC1155) to msg.sender as it's highlighted in the code above, those funds were supposed to be withdrawn only by the PrincipalToken holder (and its approved operator) :

/**
* @notice Allows principal rights owner or approved operator to withdraw the underlying token once the delegation rights have either met their expiration or been rescinded.
* Can also be called early if the caller is approved or owner of the delegate token (i.e. they wouldn't need to
* call rescind & withdraw), or approved operator of the delegate token holder
* "Burns" the delegate token, principal token, and returns the underlying tokens to the caller.
* @param delegateTokenId id of the corresponding delegate token
*/
function withdraw(uint256 delegateTokenId) external;

Tools Used

Manual review

Recommended Mitigation Steps

I recommend to check that only the PrincipalToken holder (and its approved operator) are able to withdraw to delegate token when the expiration time has passed, the revertInvalidWithdrawalConditions function can be modified as follows :

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
        );
    } else {
        // @audit make sure only PrincipalToken holder (or operator) are able the withdraw after expiration time has passed
        if (
            PrincipalToken(principalToken).isApprovedOrOwner(
                msg.sender,
                delegateTokenId
            )
        ) {
            revert Errors.NotApproved(msg.sender, delegateTokenId);
        }
    }
}