Insrt Finance security record

In the contract `ShardVaultManager`, the role `owner` has authority over the following functions: - deployShardVault(): deploy the ShardVault contract via proxy Any compromise to the `owner` account may allow a hacker to take advantage of this authority. In the contract `ShardCollection`, the role `vault` has authority over the following functions: - mint(): mint the NFT of the given ID - burn(): burn the NFT of the given ID Any compromise to the `vault` account may allow a hacker to take advantage of this authority. In the contract `ShardCollection`, the role `owner` has authority over the following functions: - addToWhitelist(): whitelists an address - removeFromWhitelist(): removes an address from whitelist - setBaseURI(): sets base URI for shard collection - setName(): sets the name for shard collection - setSymbol(): sets symbol for shard collection Any compromise to the `owner` account may allow a hacker to take advantage of this authority. In the contract `ShardVaultAdmin`, the role `owner` has authority over the following functions: - purchasePunk(): purchases a punk from CryptoPunkMarket - collateralizePunkPUSD()/collateralizePunkPETH(): borrows pUSD/pETH in exchange for collateralizing a punk - stakePUSD(): stakes an amount of pUSD into JPEGd autocompounder and then into JPEGd PUSD_CITADEL - stakePETH(): stakes an amount of pETH into JPEGd autocompounder and then into JPEGd PETH_CITADEL - investPunk(): purchases and collateralizes a punk, and stakes all pUSD gained from collateralization - setAcquisitionFee(): sets the acquisition fee BP - setSaleFee(): sets the sale fee BP - setYieldFee(): sets the yield fee BP - setMaxSupply(): sets the maxSupply of shards - setWhitelistEndsAt(): sets the whitelistEndsAt timestamp - unstakePUSD(): unstakes from JPEG'd LPFarming, then from JPEG'd citadel, then from curve LP - setReservedShards(): sets the maximum amount of shard to be minted during whitelist - unstakePETH(): unstakes from JPEG'd LPFarming, then from JPEG'd citadel, then from curve LP - setIsEnabled(): sets the isEnabled flag - initiateWhitelistAndDeposits(): sets the maximum amount of shard to be minted during whitelist, sets the whitelistEndsAt timestamp, and sets the isEnabled flag - closePunkPosition(): liquidates all staked tokens in order to pay back loan, retrieves collateralized punk - setMaxUserShards(): sets the maximum shards a user is allowed to mint - repayLoanPUSD()/repayLoanPETH(): makes a debt payment for a collateralized NFT in jpeg'd - directRepayLoanPUSD()/directRepayLoanPETH(): makes loan repayment without unstaking - listPunk(): lists a punk on CryptoPunk market place using MarketPlaceHelper contract - provideYieldPETH(): provides (makes available) yield in the form of ETH and JPEG tokens Any compromise to the `owner` account may allow a hacker to take advantage of this authority. The contracts `Core`, `ShardCollectionProxy`, and `ShardVaultDiamond` inherit `SolidStateDiamond`, where the role `owner` has authority over the following functions: - setFallbackAddress(): decides the address for the fallback implementation - diamondCut(): updates the functions callable on the diamond Any compromise to the `owner` account may allow a hacker to take advantage of this authority. The below description is based on the commit [6e64c3a819fed83344fdfb906889a62442c28096](https://github.com/Insrt-Finance/insrt-contracts/compare/ad895148af2995a6e83205de5682d190eca5f47c...6e64c3a819fed83344fdfb906889a62442c28096). The contract `MarketPlaceHelper` inherits the `Ownable` contract, where the role `owner` has authority over the following functions: - transferOwnership(): transfers ownership of the contract to a new account - renounceOwnership(): leaves the contract without owner - purchaseAsset(): purchases assets - listAsset(): lists assets - acceptAssetBid(): accepts bids of the assets - forwardSaleProceeds(): forwards the sale proceeds Furthermore, in the contract `ShardVaultAdmin`, the role `owner` has authority over the following additional functions: - setReservedSupply(): sets the maximum amount of shard to be minted during whitelist - withdrawFees(): withdraws fees to the treasury - setMaxMintBalance(): sets the maximum shards a user is allowed to mint - setAuthorized(): sets an address to be authorized or not. Authorized addresses are allowed to repay loans - stakeCard()/unstakeCard(): respectively stakes/unstakes a jpeg card - transferCard(): transfers a jpeg card to an address - makeUnusedETHClaimable(): makes any unused ETH claimable by shard holders. Note that this can be called repeatedly to artificially inflate the amount shard holders can claim - acceptPunkBid(): accepts a bid for an NFT to be sold and withdraws the proceeds The project applies the upgradable design pattern to allow dynamic upgrades of the smart contract. However, it is also worth mentioning, if the attacker takes control of the upgradability, they can steal the funds by upgrading the smart contra

The below description is based on the commit [6e64c3a819fed83344fdfb906889a62442c28096](https://github.com/Insrt-Finance/insrt-contracts/compare/ad895148af2995a6e83205de5682d190eca5f47c...6e64c3a819fed83344fdfb906889a62442c28096). The above commit deleted the invoking of `ICryptoPunkMarket(CRYPTO_PUNK_MARKET).withdraw()` to avoid the `CRYPTO_PUNK_MARKET` contract to send ETH to the `MarketPlaceHelper` contract, which does not have a `receive` function to recevie the ETH. However, the deletion of the invoking of `ICryptoPunkMarket(CRYPTO_PUNK_MARKET).withdraw()` incurs another issue that is the fund of sold punks are left in the `MarketPlaceHelper` account. ```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)']( 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, ); 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); const acceptAssetCalls = []; const encodedAcceptAssetBid = cryptoPunkMarket.interface.encodeFunctionData( 'acceptBidForPunk(uint256,uint256)', [punkId, listPrice], ); const encodedAcceptCall = { data: encodedAcceptAssetBid, target: CRYPTO_PUNKS_MARKET, value: ethers.constants.Zero, }; acceptAssetCalls.push(encodedAcceptCall); const pendingWithdrawalBefore = await cryptoPunkMarket.connect(nonOwner) ['pendingWithdrawals(address)']( marketplaceHelper.address, ); const marketplaceOwner = await marketplaceHelper.connect(nonOwner)['owner()'](); let marketOwner = await ethers.getSigner(marketplaceOwner); await hre.network.provider.request({ method: 'hardhat_impersonateAccount', params: [marketplaceOwner], }); await marketplaceHelper.connect(marketOwner) ['acceptAssetBid((bytes,uint256,address)[])']( acceptAssetCalls, ); const pendingWithdrawalAfter = await cryptoPunkMarket.connect(nonOwner) ['pendingWithdrawals(address)']( marketplaceHelper.address, ); console.log('diff', pendingWithdrawalAfter.sub(pendingWithdrawalBefore)); expect(pendingWithdrawalAfter).to.equal(BigNumber.from('100000000000000000000')); }); }); ``` Result ```terminal ShardVaultProxy ::ShardVaultProxy ::ShardVaultAdmin Unable to Withdraw Sale Proceeds diff BigNumber { value: "100000000000000000000" } ✔ lists a punk, nonOwner makes a bid, nonOwner tries to collect sale proceeds (3810ms) ``` The above test shows that the tokens of sold punks are still left in the `MarketPlaceHelper` contract, at the same time, after the delete of the invoking of the `withdraw` function, there is no way to withdraw those tokens from the `CRYPTO_PUNK_MARKET` market per the implementation of [CryptoPunksMarket](https://etherscan.io/address/0xb47e3cd837dDF8e4c57F05d70Ab865de6e193BBB#code). We also noted that there is a new `forwardSaleProceeds` function introduced, which will be triggered by `SharedVaultInternal._receivePunkProceeds()` function. However, this function has not been invoked in any other methods. Therefore, the Sold profit might never be able to be withdrawn. ```solidity function forwardSaleProceeds( EncodedCall[] memory calls ) external payable onlyOwner returns (uint256 proceeds) { for (uint256 i; i < calls.length; ) { unchecked { (bool success, ) = calls[i].target.call{ value: calls[i].value }(calls[i].data); if (!s

The `ShardVaultInternal` contract uses `MarketPlaceHelper.purchaseAsset()` to purchase NFTs that will be used as collateral, but there are no checks to ensure that the NFT has been bought and transferred to `ShardVaultInternal`. Since the `ShardVaultInternal._purchasePunk()` function updates accrued fees, it is possible for the owner to "collect" fees without actually buying assets.

The admin has the ability to close PETH punk positions through the function `closePunkPositionPETH()`, but there is no similar function for PUSD punk positions. Since `ShardVaultInternal._closePunkPosition()` is the only function that calls `INFTVault.closePosition()`, it seems that PUSD positions cannot be closed.

The `_withdrawPunkProceeds` has no check on which `punkId` is sold even though the removed `punkId` is specified by the argument: ```solidity= function _withdrawPunkProceeds(uint256 punkId) internal { ICryptoPunkMarket(PUNKS).withdraw(); ShardVaultStorage.layout().ownedTokenIds.remove(punkId); } ``` If the sold `punkId` mismatches with the specified `punkId`, the `ownedTokenIds` set will be incorrect. Furthermore, there is currently no external function that can call _withdrawPunkProceeds, meaning funds acquired from sales cannot be acquired.

The contracts cited have an unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to ambiguity when debugging, as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

In the `MarketPlaceHelper` contract, the private variable `CRYPTO_PUNK_MARKET` shall be initialized in the constructor. However, as this is an "implementation" contract of a proxy, the `CRYPTO_PUNK_MARKET` will not be initialized in the proxy contract. Additionally, the `CRYPTO_PUNK_MARKET` variable is not used in within the contract.