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

Missing address(0) Check in `notifyAccountTransfer` Function Leading to Potential Token Lock

Fortis Audits

Summary

The notifyAccountTransfer function in the Account NFT contract lacks a check for to != address(0). This oversight allows the possibility of the owner of a trading account being set to address(0), which can subsequently lead to the inability to deposit or withdraw from that account. This vulnerability can cause tokens to become irretrievably stuck in the contract.

Vulnerability Details

The notifyAccountTransfer function is designed to update the owner of a trading account when an account transfer occurs. However, it does not validate the to parameter to ensure it is not address(0). As a result, if the to address is set to address(0), the owner of the trading account becomes address(0).

In other parts of the contract, specifically in the deposit and withdraw functions, there are checks to determine if the owner of a token ID is address(0). If the owner is address(0), the account is considered non-existent, and no deposits or withdrawals can occur. This discrepancy can lead to a situation where tokens are stuck in the contract, as the new owner (now address(0)) cannot perform any actions on the account.

Code Snippet

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

// @audit : missing to!=address(0) check
function notifyAccountTransfer(address to, uint128 tradingAccountId) external {
_onlyTradingAccountToken();
​
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
tradingAccount.owner = to;
}```

Impact

The impact of this vulnerability is significant:

  • Token Lock: If the owner of a trading account is set to address(0), the tokens associated with that account become stuck in the contract, as no deposits or withdrawals can be made.

  • Loss of Access: Users may lose access to their tokens if the account ownership is inadvertently or maliciously set to address(0).

  • Contract Inconsistency: The contract's logic for handling accounts becomes inconsistent, potentially leading to further issues in contract functionality and user trust.

POC

function testFuzz_Fortis_account_transfer_Tozeroaddress(uint256 marginValueUsd) external {
changePrank({ msgSender: users.naruto.account });
marginValueUsd = bound({
x: marginValueUsd,
min: WSTETH_MIN_DEPOSIT_MARGIN,
max: convertUd60x18ToTokenAmount(address(wstEth), WSTETH_DEPOSIT_CAP_X18)
});
deal({ token: address(wstEth), to: users.naruto.account, give: marginValueUsd });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsd, address(wstEth));
​
changePrank({ msgSender: address(tradingAccountToken) });
// @audit : account is transfer to address(0) .
perpsEngine.notifyAccountTransfer(address(0), tradingAccountId);
// old user cannot withdraw
changePrank({ msgSender: users.naruto.account });
perpsEngine.withdrawMargin(tradingAccountId, address(wstEth), marginValueUsd);
// new user cannot withdraw
changePrank({ msgSender: address(0) });
perpsEngine.withdrawMargin(tradingAccountId, address(wstEth), marginValueUsd);
}
​

Tools Used

Manual review , Foundry

Recommendations

To mitigate this vulnerability, it is essential to add a check in the notifyAccountTransfer function to ensure that the to address is not address(0) before updating the owner of the trading account.

function notifyAccountTransfer(address to, uint128 tradingAccountId) external {
_onlyTradingAccountToken();
++ require(to!=address(0));
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
tradingAccount.owner = to;
}```
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 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.