Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

`AccountNFT.mint` always revert with `Errors.AccountNotFound`.

qpzm

Summary

AccountNFT.mint always revert with Errors.AccountNotFound.

Vulnerability Details

AccountNFT overrides _update to sync tradingAccount.owner with the NFT owner.
https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/account-nft/AccountNFT.sol#L23-L25

function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
address previousOwner = super._update(to, tokenId, auth);
// @audit
IPerpsEngine(owner()).notifyAccountTransfer(to, tokenId.toUint128());

However, notifyAccountTransfer calls loadExisting to load the previous trading account.
https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/branches/TradingAccountBranch.sol#L408

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

The problem is that a previous owner is address(0) when minting a new NFT. Therefore, AccountNFT.mint always reverts with Errors.AccountNotFound.
https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/leaves/TradingAccount.sol#L69-L74

function loadExisting(uint128 tradingAccountId) internal view returns (Data storage tradingAccount) {
tradingAccount = load(tradingAccountId);
if (tradingAccount.owner == address(0)) {
revert Errors.AccountNotFound(tradingAccountId, msg.sender);
}
}

POC

  1. Add the following test function test_AccountNFT_Mint in [NotifyAccountTransfer_Integration_Test]
    (https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/test/integration/perpetuals/trading-account-branch/notifyAccountTransfer/notifyAccountTransfer.t.sol#L13).

  2. Run forge test --mt test_AccountNFT_Mint.

contract NotifyAccountTransfer_Integration_Test is Base_Test {
// ...
​
// @audit Add this test
function test_AccountNFT_Mint() external {
uint256 tokenId = 1;
address owner = tradingAccountToken.owner();
vm.startPrank(owner);
vm.expectRevert(abi.encodeWithSelector(Errors.AccountNotFound.selector, tokenId, address(tradingAccountToken)));
tradingAccountToken.mint(address(this), tokenId);
}
​
// ...
}

Impact

AccountNFT contract cannot mint any NFTs.

Tools Used

Manual, Foundry

Recommendations

Replace loadExisting with load in notifyAccountTransfer.
https://github.com/Cyfrin/2024-07-zaros/blob/7439d79e627286ade431d4ea02805715e46ccf42/src/perpetuals/branches/TradingAccountBranch.sol#L408

- TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
+ TradingAccount.Data storage tradingAccount = TradingAccount.load(tradingAccountId);

Reason

notifyAccountTransfer is only allowed to AccountNFT, and AccountNFT only calls notifyAccountTransfer in _update function.
Therefore, loadExisting in notifyAccountTransfer reverts only when tradingAccount.owner is address(0), or
equivalently when AccountNFT is newly minted. In conclusion, load is more suitable than loadExisting in
notifyAccountTransfer.

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!