receiving funds through `LSP1UniversalReceiverDelegateUP` can fail

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP5ReceivedAssets/LSP5Utils.sol#L74-L83 https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol#L180-L181

Vulnerability details

Impact

In certain wrongly configured circumstances a contract can fail to receive funds. Either if the storage is malformed or if the LSP1UniversalReceiverDelegateUP contract has the wrong permissions. Permissions can be fiddly to setup, especially if the goal is to keep with best practice and only allow least possible permissions.

Hence if in any way this is wrong the account will fail to receive funds. If the sender doesn't resend this the account has missed out on this forever.

Proof of Concept

When receiving assets in LSP1UniversalReceiverDelegateUP there are a couple of steps that can fail, first one being if the receivedAssetsCount is malformed:

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP5ReceivedAssets/LSP5Utils.sol#L74-L83

File: LSP5ReceivedAssets/LSP5Utils.sol

74:        bytes memory encodedArrayLength = getLSP5ReceivedAssetsCount(account);
75:
76:        // CHECK it's either the first asset received,
77:        // or the storage is already set with a valid `uint128` value
78:        if (encodedArrayLength.length != 0 && encodedArrayLength.length != 16) {
79:            revert InvalidLSP5ReceivedAssetsArrayLength({
80:                invalidValueStored: encodedArrayLength,
81:                invalidValueLength: encodedArrayLength.length
82:            });
83:        }

And later if the LSP1UniversalReceiverDelegateUP contract fails to write the keys to the account storage:

https://github.com/code-423n4/2023-06-lukso/blob/main/contracts/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol#L180-L181

File: LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol

180:            // Set the LSP5 generated data keys on the account
181:            IERC725Y(msg.sender).setDataBatch(dataKeys, dataValues);

This fails if the contract lacks the necessary permissions.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider not reverting on malformed storage and doing the storage call in a try. If any of these fails, an event can be emitted and the erroneous configuration/storage can be fixed manually.

If any of these fail, the impact will be that the internal accounting of receiving these funds will be off, as the record of these could not be written. This is favorable to not receiving them at all as the storage can be manually fixed by the owner and made to reflect the current accounting of the contract.

Assessed type

Token-Transfer

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Insufficient proof

[c4-judge]

trust1995 marked the issue as unsatisfactory: Insufficient proof