Papr contest - rotcivegaf's results

Lines of code

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

Vulnerability details

Author: rotcivegaf

Impact

The owner of the ERC721 token could approve an operator to manage his tokens With the misunderstanding of operator with from in the onERC721Received function the benefits of this function goes to the operator instead of the from(owner):

• _addCollateralToVault(from, collateral); the operator can remove the collateral(NFT) instead of the original owner
• _increaseDebtAndSell(from, request.proceedsTo, ERC721(msg.sender), request.swapParams, request.oracleInfo); increasing the debt and sell to the operator instead of the original owner
• _increaseDebt(from, collateral.addr, request.proceedsTo, request.debt, request.oracleInfo); increasing the debt of the operator instead of the original owner

Note: read the interface ERC721TokenReceiver in EIP721

Proof of Concept

• Alice set approve to Kane to manage his NFT (setApprovalForAll or approve)
• Kane send the alice NFT to the contract PaprController
• When the contract PaprController receive the NFT, the function onERC721Received call the internal functions _addCollateralToVault and/or _increaseDebtAndSell or _increaseDebt with the from as the operator parameter
• Alice can't call removeCollateral because Kane(the operator) it's the collateralOwner, setted in collateralOwner[collateral.addr][collateral.id] = account;
• All benefits to deposit the collateral goes to Kane(operator) instead of Alice(the owner)

Add this in the test contract OnERC721ReceivedTest:

    function testOnERC721ReceivedWithOperator() public {
        vm.startPrank(borrower);
        safeTransferReceivedArgs.swapParams.sqrtPriceLimitX96 = _maxSqrtPriceLimit(true);

        address alice = address(2);
        nft.approve(alice, collateralId);

        vm.stopPrank();
        vm.startPrank(alice);
        nft.safeTransferFrom(borrower, address(controller), collateralId, abi.encode(safeTransferReceivedArgs));

        // This should be pass
        IPaprController.VaultInfo memory vaultInfo = controller.vaultInfo(borrower, collateral.addr);
        assertEq(vaultInfo.count, 1);
        assertEq(vaultInfo.debt, debt);
    }

Tools Used

Review

Recommended Mitigation Steps

@@ -156,7 +156,7 @@ contract PaprController is
     /// @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)
+    function onERC721Received(address, address from, uint256 _id, bytes calldata data)
         external
         override
         returns (bytes4)