code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/bd49f57c32a522563fc42feeee23c83c8b373405/contracts/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol#L78-L81

Vulnerability details

Background

One way to solve this problem is by creating a standard and unified function that any smart contract can implement. DEXs, Wallets, or profiles could use this function to be notified about an incoming asset, information, followers, etc.

Impact

If the owner of contract A wishes to authorize a user to send information or notify other contracts on behalf of A, they would grant the user permission to call function universalReceiver of other contracts (on behalf of A). However, the user may use this permission in an unintended way by directly calling function universalReceiver in the UniversalReceiverDelegateUP contract (on behalf of A). This could lead to arbitrary manipulation of A's storage, even if the user does not have permission to set data.

Proof of Concept

// in contract A
function universalReceiver(
    bytes32 typeId,
    bytes calldata receivedData
) public payable virtual returns (bytes memory returnedValues) {
        ......
        if (universalReceiverDelegate.supportsERC165InterfaceUnchecked(_INTERFACEID_LSP1)) {
            resultDefaultDelegate = universalReceiverDelegate
                .callUniversalReceiverWithCallerInfos(
                    typeId,
                    receivedData,
                    msg.sender,
                    msg.value
                );
        }
        ......
}

function callUniversalReceiverWithCallerInfos(
    address universalReceiverDelegate,
    bytes32 typeId,
    bytes calldata receivedData,
    address msgSender,
    uint256 msgValue
) internal returns (bytes memory) {
    bytes memory callData = abi.encodePacked(
        abi.encodeWithSelector(
            ILSP1UniversalReceiver.universalReceiver.selector,
            typeId,
            receivedData
        ),
        msgSender,
        msgValue
    );

    (bool success, bytes memory result) = universalReceiverDelegate.call(callData);
    ......
}

// in contract UniversalReceiverDelegateUP
function universalReceiver(
    bytes32 typeId,
    bytes memory /* data */
) public payable virtual returns (bytes memory result) {
    if (msg.value != 0) revert NativeTokensNotAccepted();

    // This contract acts like a UniversalReceiverDelegate of an LSP0ERC725Account where we append the
    // address and the value, sent to the universalReceiver function of the LSP0, to the msg.data
    // Check https://github.com/lukso-network/LIPs/blob/main/LSPs/LSP-0-ERC725Account.md#universalreceiver
    address notifier = address(bytes20(msg.data[msg.data.length - 52:]));
    ......
}

The correct call path should be universalReceiver(A) => callUniversalReceiverWithCallerInfos => universalReceiver(delegate), and notifier will be the caller of universalReceiver(A). However, a user with permission can directly call universalReceiver(delegate) in UniversalReceiverDelegateUP (on behalf of A), which means msg.data can be arbitrary. This allows them to impersonate any notifier and manipulate the corresponding storage.

In current implementaion, thanks to the balance check, impersonating a LSP7 or LSP8 contract does not work. However, a malicious user can impersonate a LSP9 Vault to concoct a _TYPEID_LSP9_OwnershipTransferred_SenderNotification or _TYPEID_LSP9_OwnershipTransferred_RecipientNotification. As a result, the storage of received vaults could be manipulated, such as deleting existing vaults or adding nonexistent vaults, even if the user is not allowed to set data.

Tools Used

Manual Review

Recommended Mitigation Steps

Rename the receiver function in UniversalReceiverDelegateUP

function universalReceiverDelegate(
bytes32 typeId,
bytes memory /* data */
) public payable virtual returns (bytes memory result)

Warn users that granting permission to call universalReceiverDelegate is dangerous

Assessed type

Access Control

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

LSP9 Vault is OOS

trust1995 commented 1 year ago

However, a user with permission can directly call universalReceiver(delegate) in UniversalReceiverDelegateUP (on behalf of A), which means msg.data can be arbitrary

When directly calling, the delegate will change the data of the caller, as seen below. Therefore, the issue is invalid.

    IERC725Y(msg.sender).setData(dataKeys, dataValues);
    return "";
} else {
    (dataKeys, dataValues) = LSP10Utils.generateReceivedVaultKeys(
        msg.sender,
        notifier,
        notifierMapKey
    );
    IERC725Y(msg.sender).setData(dataKeys, dataValues);
    return "";
}

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Invalid

xuwinnie commented 1 year ago

However, a user with permission can directly call universalReceiver(delegate) in UniversalReceiverDelegateUP (on behalf of A), which means msg.data can be arbitrary

Here I don't mean user directly call universalReceiver(delegate), what i want to express is user(with permission) call execute at LSP0Account (of contract A) and target is universalReceiver(delegate). So msg.sender here will be Contract A. Sorry for the confusion.

skimaharvey commented 1 year ago

Not valid imo:

We have a very refined permissions setting when it comes to CALL and there is no reason to give someone this specific call permission to the URD, if you do it is intended so not a bug
if the caller has the SUPER_CALL permission, he can do way worse than manipulating the storage of the UP like rugging all your assets within the vault or transferring the vault to an address you own
vaults are out of scope and will be totally re-standardized