Papr contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The collateral is assigned to the operator's vault because of a parameter mismatch. This impacts the ability of third parties to integrate the PaprController contract. You're not able to create an intermediary contract that adds collateral to a user's vault. It has to be the user themselves sending the NFT directly else the collateral is assigned to the calling contract (operator). If the collateral is assigned to the NFT owner's vault you can have two types of integrations:

• the intermediary contract has its own vault and handles all the user logic itself (user approved NFT to intermediary contract -> contract transfers NFT to itself -> contract sends NFT to PaprController to add the collateral to their vault)
• intermediary contract works with the user's own vault (user approves NFT to intermediary contract -> contract transfers NFT to PaprController to add to user's vault)

Only the first integration type is possible in the current form because the collateral is always assigned to the operator (intermediary contract).

Generally, I believe that the collateral being assigned to the operator is unexpected behavior that might cause headaches if the caller doesn't read the contracts properly. To undo that, the operator has to pay back the debt, remove the collateral and send it back to the user. Depending on the contract used to transfer the collateral in the first place, that might not be possible. In that rare scenario that might cause a loss of funds.

Proof of Concept

The onERC721Received() function has the following interface

function onERC721Received(address _operator, address _from, uint256 _tokenId, bytes _data) external returns(bytes4);

The PaprController contract expects the first parameter to be the NFT owner, although that's the operator's address (see NATSPEC comment):

    /// @notice Handler for safeTransferFrom of an NFT
    /// @dev Should be preferred to `addCollateral` if only one NFT is being added
    /// to avoid approval call and save gas
    /// @param from the current owner of the nft
    /// @param _id the id of the NFT
    /// @param data encoded IPaprController.OnERC721ReceivedArgs
    /// @return selector indicating succesful receiving of the NFT
    function onERC721Received(address from, address, uint256 _id, bytes calldata data)
        external
        override
        returns (bytes4)
    {
        IPaprController.OnERC721ReceivedArgs memory request = abi.decode(data, (IPaprController.OnERC721ReceivedArgs));

        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;
    }

Tools Used

none

Recommended Mitigation Steps

Use the owner's address instead of the operator's.