code-423n4 / 2023-06-lukso-findings

3 stars 1 forks source link

Potential Reversion in Transfer due to LSP1 Interface Support Check #95

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAssetCore.sol#L454-L469 https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol#L452-L466

Vulnerability details

SORRY I HAVFE PREVIOUSLY SUBMITTED THIS ISSUE WITHOUT THE FIX... (FIRST TIME WARDEN FORGIVE ME)

Impact

The transfer function in LSP7DigitalAssetCore & LSP8DigitalAssetCore includes a mandatory hook, _notifyTokenSender, which verifies if the sender supports _INTERFACEID_LSP1. However, if a token owner who initially implemented LSP1 interface ceases to support it, this verification fails, causing an error in the transfer function.

This is a significant issue as it can interrupt token transfers in situations like DeFi protocols, where a user might have granted permissions to a swapping pool. For example, an old UniversalProfile (UP) owner could have set a Universal Receiver Delegate under _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY, which tries to call back setData on the UP. If the UP's new owner is an Externally Owned Account (EOA) that has not modified all keys of the old owner, this callback would fail, causing the transfer function to revert, disrupting swaps or other operations.

Proof of Concept

This issue could occur when a UP changes its ownership from a contract (e.g., LSP6KeyManager) to an EOA. In such a case, the old Universal Receiver Delegate would attempt to call setData on the UP during the execution of the _notifyTokenSender hook, which would fail because EOAs don't support this function. This would cause a reversion in the transfer function, disrupting operations.

Tools Used

The issue was identified through manual code review without the use of specific security tools.

Recommended Mitigation Steps

Implement a try-catch block within the _notifyTokenSender function to ensure the function doesn't revert if the call fails. This approach can mitigate the immediate risk.

Assessed type

DoS

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

CJ42 marked the issue as disagree with severity

c4-sponsor commented 1 year ago

CJ42 requested judge review

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Insufficient proof