The team of Float contracted byterocket to conduct a smart contract audit of smart contracts related to a new cross-chain NFT project called “A.P. Morgan Sailing Club”. Float is a “peer-to-peer, yield-enhanced, floating synthetic asset exposure mechanism”. They describe themselves as “the easiest and safest way for users to buy synthetic assets. Users do not need to worry about over-collateralization, or suddenly getting liquidated”.
The team of byterocket reviewed and audited the above smart contracts in the course of this audit. We started on the 5th of July and finished on the 26th of July 2022. A subsequent update of the report took place on the 31st of October 2022.
byterocket gained access to the code via a private GitHub repository. We based the audit on the main branch’s state on July 8th, 2022 (commit hash 962aa57df35354e85a6aed52ba51ec6b028bab95). The updated version was provided to us via multiple new commits to the repository, addressing our findings. The last and most recent commit hash that we audited is c347dca2284af7d6310e131881116dc90c9275f7 from August 15th, 2022.
We conducted a manual multi-pass code review of the smart contracts mentioned in section (1). Three different people went through the smart contract independently and compared their results in multiple concluding discussions.
The manual review and analysis were additionally supported by multiple automated reviewing tools, like Slither, GasGauge, Manticore, and different fuzzing tools.
We are categorizing our findings into four different levels of severity:
On the code level, we found 9 bugs or flaws, with 9 of them being fixed in a subsequent update. Prior to this, there have been 5 non-critical, 1 of low severity, 2 of medium severity, and 1 of high severity findings. Our automated systems and review tools did not find any additional ones. Additionally, we found 2 gas improvements, which have also been addressed in a subsequent update.
The contracts are written according to the latest standard used within the Ethereum community and the Solidity community’s best practices. The naming of variables is very logical and understandable, which results in the contract being easy to understand. The code is sufficiently documented.
Location: APMorganMinter.sol - Line 180 - 185
Description:
The function is intended to withdraw a number of $LINK tokens held in the contract. The contract holds and utilizes $LINK tokens as payment for the VRF functionality. However, the contract also has a publicly-callable topUpSubscription() function that transfers a number of $LINK tokens to the VRFCoordinator to top up the payment for the VRF functionality. Therefore, any withdrawal transaction can be front-run with a topUpSubscription() call. Already paid $LINK tokens can only be withdrawn by canceling the VRF subscription. This is done via the only-callable-by-admin cancelSubscription() function. However, once a subscription gets canceled, the contract provides no functionality to create a new subscription (subscription is created in the initialize() function). Therefore, every $LINK token held in the contract could (by a griefing attacker) be deposited into the VRF subscription without the possibility of withdrawing them (without stopping the mint functionality).
These are the affected lines of code:
Recommendation:
In order to disable the frontrunning capability, the topUpSubscription() function could be made “onlyAdmin”.
It’s also possible to create an external “onlyAdmin” createSubscription() function. This would not disable the griefing attack vector but enables the admin to regain the overpaid LINK tokens (by canceling the subscription) while not disabling the mint functionality (by creating a new subscription).
A third, arguably less desirable, method would be to send every withdraw() transaction through a private mempool, e.g. flashbots.
Update on the 31st of October 2022:
The developers have fixed this issue by only allowing the DEFAULT_ADMIN_ROLE to execute the topUpSubscription() function.
Location: APMorgan.sol - Line 461 - 480
Description:
The second check in the first if-clause should be preferredToken[from] == tokenId. This way, the user’s preferred token is only updated if it was the token that was sent. Otherwise, the user’s preferred token is changed even if the user sends a different token.
These are the affected lines of code:
Recommendation:
Consider adapting the second check in the if-clause to conform to the described logic of the function, fixing the issue.
Update on the 31st of October 2022:
The developers have updated the logic of the AfterTransferHook according to our recommendation, fixing this issue.
Location: lzApp/LzAppUpgradeable.sol - Line 155 - 162
Description:
The function accepts a bytes calldata as _srcAddress (Note that this is due to LayerZero’s Interface). However, the address read from trustedRemoteLookup has to be correct in the transferCrossChain() function. The function itself can not check that the address is correct, because the contract of the address is on a different chain.
If the asynchronous mint on the other chain fails, due to an invalid destination address, the burn on the original chain was already executed anyway, leading to a loss of that NFT.
Furthermore, there is no specification whether the _srcAddress is given in 32 bytes or 20 bytes, i.e. abi.encode(address(0x...)) or abi.encodePacked(address(0x...)).
These are the affected lines of code:
Recommendation:
Consider specifying the size of the bytes stored in trustedRemoteLookup and checking that the _srcAddress given in setTrustedRemote() has that specific size, i.e. require(_srcAddress.length == {32|20}, "Unexpected format");.
Update on the 31st of October 2022:
The developers have added a require statement to the function, ensuring that the _stcAddress parameter has a length of 20 bytes.
Location: APMorgan.sol - Line 510 - 515
Description:
The setPreferredToken() function lets a user set a preferred token if they have the approval of that token. However, as there is no callback implemented after a token approval has changed, it is possible for many users to have the same preferred token, while none of them owns them.
Consider the following example:
Let user A own token 1 → user A approves token 1 to user B →user B sets token 1 as preferredToken → user A approves token 1 to user C → user C sets token 1 as preferredToken → …
These are the affected lines of code:
Recommendation:
Consider either only allowing the current owner to set a token as their preferred token or using any other method to ensure that users may not set unowned tokens as their preferred token.
Update on the 31st of October 2022:
The developers have updated the corresponding function, swapping the msg.sender statement with an ownerOf(tokenId), so even if an approved address calls this function, it will only set the preferred token of the owner accordingly.
Location: APMorgan.sol - Line 306, 316, 326 & 351
Description:
The two functions _mintAPMorgan() as well as _mintCrossChain() have a typo with the word ”receiver” being ”reciever”.
These are the affected lines of code:
Recommendation:
Consider correcting the parameter to be ”receiver” instead of ”reciever”.
Update on the 31st of October 2022:
The developers have fixed the typo accordingly.
Location: APMorgan.sol - Line 139 - 140
Description:
It is best practice to write numerals that are 1,000 or higher with underscores, like so:
This highly increases its readability.
These are the affected lines of code:
Recommendation:
Consider making use of underscores to increase the readability of number literals like so:
Update on the 31st of October 2022:
The developers have updated the hardcoded literals in the code to conform to the best practice.
Location: APMorgan.sol - Line 156 - 157
Description:
The initialize() function receives a startIndex and endIndex argument. They are not verified though, so the endIndex could be less than the startIndex, which would lead to issues.
Recommendation:
Consider adding a check, verifying that startIndex < endIndex.
Update on the 31st of October 2022:
The developers have acknowledged this finding, but chose not to change their implementation due to contract size limitations - which is fine as this does not pose any immediate risks if the contracts are deployed and initialized properly.
Location: APMorgan.sol - Line 55 & 156
Description:
The startIndex variable, which defines the first tokenId of minted NFTs, is not used beyond the initialize() function.
Recommendation:
Consider either using the variable or removing it altogether if it is not required.
Update on 31st of October 2022:
The developers have acknowledged this finding but chose not to change their implementation due to their off-chain mechanisms relying on this value existing on-chain - which is fine as this does not pose any risks.
Location: APMorgan.sol - Line 548
Description:
As the tokenId is incremented after the require-statement, which contains a less-than-equal instead of a less-than, the tokenIdCounter will be incremented one last time once the endIndex has been reached. From this point onwards, the getTokenIdCounter() function will always return endIndex + 1, which might be misleading.
These are the affected lines of code:
uint256 tokenId = _tokenIdCounter.current();
require(tokenId <= endIndex, "Max supply reached for chain!");
_tokenIdCounter.increment();
Recommendation:
Consider updating the documentation of the getTokenIdCounter() function to reflect that it does not return the latest tokenId but the next one instead.
Update on the 31st of October 2022:
The developers have updated the documentation of the function accordingly.
Description:
The function fulfillRandomWords() is called by the Chainlink VRC asynchronous callback. It mints a token and the token data is read from the premintedTokens mapping.
As the mapping is not used afterward anymore, consider deleting the data after a successful mint, i.e.:
Description:
The variable is set to a hardcoded value once during the initialize() function. Consider making it a constant to save some gas.
Part of our audits are also analyses of the protocol and its logic. The byterocket team went through the implementation and documentation of the implemented protocol.
The repository itself contained tests and documentation. We found the provided unit tests that are coming with the repository execute without any issues and cover the most important parts of the protocol.
According to our analysis, the protocol and logic are working as intended, given that any findings with a severity level are fixed.
We were not able to discover any additional problems in the protocol implemented in the smart contract.
During our code review (which was done manually and automated), we found 9 bugs or flaws, with 11 of them being fixed in a subsequent update. Prior to this, there have been 5 non-critical, 1 of low severity, 2 of medium severity, and 1 of high severity findings. Our automated systems and review tools did not find any additional ones. Additionally, we found 2 gas improvements, which have also been addressed in a subsequent update.
The protocol review and analysis did neither uncover any game-theoretical nature problems nor any other functions prone to abuse.
In general, we are happy with the overall quality of the code and its documentation. The developers have been very responsive and were able to answer any questions that we had.
As of the date of publication, the information provided in this report reflects the presently held understanding of the auditor’s knowledge of security patterns as they relate to the client’s contract(s), assuming that blockchain technologies, in particular, will continue to undergo frequent and ongoing development and therefore introduce unknown technical risks and flaws. The scope of the audit presented here is limited to the issues identified in the preliminary section and discussed in more detail in subsequent sections. The audit report does not address or provide opinions on any security aspects of the Solidity compiler, the tools used in the development of the contracts or the blockchain technologies themselves, or any issues not specifically addressed in this audit report.
The audit report makes no statements or warranties about the utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, the legal framework for the business model, or any other statements about the suitability of the contracts for a particular purpose, or their bug-free status.
To the full extent permissible by applicable law, the auditors disclaim all warranties, express or implied. The information in this report is provided “as is” without warranty, representation, or guarantee of any kind, including the accuracy of the information provided. The auditors hereby disclaim, and each client or user of this audit report hereby waives, releases and holds all auditors harmless from, any and all liability, damage, expense, or harm (actual, threatened, or claimed) from such use.
We store our public audit reports on IPFS; a peer-to-peer network called the "Inter Planetary File System". This allows us to store our reports in a distributed network instead of just a single server, so even if our website is down, every report is still available.
The IPFS Hash, a unique identifier of the report, is signed on-chain by both the client and us to prove that both sides have approved this audit report. This signing mechanism allows users to verify that neither side has faked or tampered with the audit.
