c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

There is no access control for the function rescind, thus 1) it lets anyone rescind any token, either he is the owner or not and 2) the documentation does not adhere to the current implementation.

Proof of Concept

The documentation above the definition of DelegateToken::rescind in the interface IDelegateToken says:

    /**
     * @notice Allows the delegate owner or any approved operator to return a delegate token to the principal rights holder early, allowing the principal rights holder to redeem
     * the underlying token(s) early. Allows anyone to forcefully return the delegate token to the principal rights holder if the delegate token has expired
     * @param delegateTokenId Which delegate right to rescind
     */
    function rescind(uint256 delegateTokenId) external;

so it is expected to

either the owner or an operator executes the whole function at any time
after the delegate token has expired, anyone can rescind it

However, in its current implementation we see that, although there is a check for the expiry, the execution flow continues to the transferFrom without problems FOR ANYONE, either the owner or a malicious actor who wants to break the contract behavior.

    function rescind(uint256 delegateTokenId) external {
        //slither-disable-next-line timestamp
        if (StorageHelpers.readExpiry(delegateTokenInfo, delegateTokenId) < block.timestamp) {
            StorageHelpers.writeApproved(delegateTokenInfo, delegateTokenId, msg.sender);
            // approve gets reset in transferFrom or this write gets undone if this function call reverts
        }
        transferFrom( <============================================ HERE no checks for msg.sender to be the owner or an approved operator, as the documentation suggests
            RegistryHelpers.loadTokenHolder(delegateRegistry, StorageHelpers.readRegistryHash(delegateTokenInfo, delegateTokenId)),
            IERC721(principalToken).ownerOf(delegateTokenId),
            delegateTokenId
        );
    }

As he can attack anyone by passing an arbitrary delegateTokenId and rescind any token at any time (breaking one of the core pillars of the contract), I consider it as a high.

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider implementing the checks for msg.sender to be a trusted one BEFORE the delegate token is expired. After the deadline is met, just make it possible to anyone to rescind the token.

TLDR -> implement correctly the function as the documentation suggests

Assessed type

Access Control

c4-judge commented 11 months ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

GalloDaSballo commented 11 months ago

Anyone can send the token back as described in the code