Zaros Part 1

Summary

The NFT transfer process involves updating ownership in both the TradingAccount structure and the NFT token. However, a missing function call to _update results in inconsistent ownership updates. While the owner field of the TradingAccount structure is updated, the owner of the NFT token remains unchanged, leading to discrepancies in the protocol's internal data structures.

Vulnerability Details

• Function Definition: The _update function is designed to handle the transfer of NFTs by updating the NFT token's owner.

• Issue: The notifyAccountTransfer function updates the owner field of the TradingAccount structure but does not invoke _update, leaving the NFT token's ownership unchanged.

• Impact: This oversight results in a mismatch between the on-chain ownership data and the protocol's internal data structures, leading to potential inconsistencies and operational issues.

Code Snippet

`https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/TradingAccountBranch.sol#L400-L410`

Impact

The inconsistency in ownership updates can lead to significant issues, such as:

• Mismatch between on-chain and protocol data: This discrepancy can cause confusion and potential disputes over the true ownership of NFTs.

• Operational Risks: The protocol's functionality may be compromised, leading to errors in account management and transfer operations.

• User Trust: Users may lose trust in the protocol's reliability and accuracy, potentially affecting user retention and adoption.

POC

Add this function in ERC721.sol

Add this external function in AccountNFT.sol

Logs

Tools Used

Manual Review .

Recommendations

To resolve this issue and ensure consistency between on-chain ownership and the protocol's internal data structures, the _update function should be properly invoked during the account transfer process.

1. Modify Function Scope: Make an external update function in AccountNFT.sol

   function update(address to, uint256 tokenId, address auth) external returns (address) {

   address previousOwner = super._update(to, tokenId, auth);

   return previousOwner;

   }

   `

2. Ensure Consistent Calls: Verify that notifyAccountTransfer includes a call to update to maintain consistency across all ownership data and add an extra argument auth` in `notifyAccountTransfer` .

   `

   ++ function notifyAccountTransfer(address to, uint128 tradingAccountId , address auth ) external {

   _onlyTradingAccountToken();

   ++ GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();

   ​

   ++ IAccountNFT tradingAccountToken = IAccountNFT(globalConfiguration.tradingAccountToken);

   ​

   TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);

   tradingAccount.owner = to;

   ++ tradingAccountToken.update(to, tradingAccountId, auth );

   ​

   }

   ​

   `