Papr contest - Koolex's results

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L159

Vulnerability details

Impact

To add a collateral, one could send the NFT directly to the contract, onERC721Received is called then to handle adding the collateral to the vault. https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L159-L177

However, if the user sends the NFT via an operator then the ownership of the collateral will be assigned to the operator instead of the user. This results in getting the NFT stuck in the protocol permanently since in this case the collateral can be removed only by the operator and not the user.

Proof of Concept

According to EIP721 specification, the first parameter of onERC721Received is supposed to be the operator and the second is the previous owner. https://eips.ethereum.org/EIPS/eip-721#specification

However, the first parameter of onERC721Received's implementation here is named "from" and used to assign the ownership of the collateral while the second parameter is just neglected.

    function onERC721Received(address from, address, uint256 _id, bytes calldata data)
        external
        override
        returns (bytes4)
    {
        IPaprController.OnERC721ReceivedArgs memory request = abi.decode(data, (IPaprController.OnERC721ReceivedArgs));
        // msg.sender = address of NFT collection
        IPaprController.Collateral memory collateral = IPaprController.Collateral(ERC721(msg.sender), _id);

        _addCollateralToVault(from, collateral);

        if (request.swapParams.minOut > 0) {
            _increaseDebtAndSell(from, request.proceedsTo, ERC721(msg.sender), request.swapParams, request.oracleInfo);
        } else if (request.debt > 0) {
            _increaseDebt(from, collateral.addr, request.proceedsTo, request.debt, request.oracleInfo);
        }

        return ERC721TokenReceiver.onERC721Received.selector;
    }

To get a sense of how onERC721Received function is called, check this code from solmate's ERC721 implementation:

    function safeTransferFrom(
        address from,
        address to,
        uint256 id,
        bytes calldata data
    ) public virtual {
        transferFrom(from, to, id);

        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
    }

https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC721.sol#L126-L140

Tools Used

Manual analysis

Recommended Mitigation Steps

Use the second parameter as "from" instead of the first.