Possible malicious/unintended code execution from delegatecall leading to unexpected consequences, countermeasures needed

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/LooksRareAggregator.sol#L88

Vulnerability details

Impact

The main aggregator uses delagatecall on proxy's function. The delegatecall will then fulfill all transactions on the relevant market place, which is quite complecated. Although proxies are white listed, it is still possible to make unintended state changes on the main aggregator. It is also possible that some malicious code is excuted to change main aggregator's state variables. Measures should be taken to reduce the risk.

Proof of Concept

In the below code, for brevity, we assume Contract LooksRareAggregator uses delegatecall on the Proxy MaliciousProxy to fulfill transactions. The LooksRareAggregator's owner state variable is assumed to be stored in Slot 3. After the delegatecall _delegateCallToMaliciousProxy(), which is executed by LooksRareAggregator's execute() function, the value of LooksRareAggregator's owner is altered.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract LooksRareAggregator {
    // slot 0
    uint256 public slot0_num;
    address public slot1_var;
    uint256 public slot2_var;
    // slot 3 - owner
    address public owner;
    // ... other state vars
    address public maliciousProxy;

    constructor(address _maliciousProxy) {
        owner = msg.sender;
        maliciousProxy = _maliciousProxy;
    }

    function execute() external {
        // ... some code
        _delegateCallToMaliciousProxy();
       // ...other code
    }

    function _delegateCallToMaliciousProxy() internal {
        bytes memory someTradeData = abi.encodeWithSignature("execute()");
        (bool success, ) = maliciousProxy.delegatecall(someTradeData);
        require(success);
        // ... other code
    }
}

contract MaliciousProxy {
    function execute() external {
        // suppose `owner` is stored in slot `OWNER_SLOT`
        uint256 OWNER_SLOT = 3;
        address intendedMaliciousOwnerAddress = 0x7EF2e0048f5bAeDe046f6BF797943daF4ED8CB47;

        assembly {
            sstore(OWNER_SLOT, intendedMaliciousOwnerAddress)
        }
    }
}

Tools Used

Manual audit.

Recommended Mitigation Steps

To prevent delegatecall from changing state variables, we can add require() statements before and after the delegatecall to make sure values of important state vairables (e.g. owner, and other important state variables) are not altered after proxy delegatecall. For example, we change LooksRareAggregator.sol#L88 to:

address oriOwner = owner;
// other important state vairables like base point, fee recipient
(bool success, bytes memory returnData) = singleTradeData.proxy.delegatecall(proxyCalldata);  // L88
require(oriOwner == owner); // to make sure the value of `owner` is not altered
// require(values of other important state variables are not altered)

[c4-judge]

Picodes marked the issue as duplicate of #125

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-11-looksrare-findings/issues/199