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

Grouped Low Submissions

krisrenzo

Incompactibility between LiquidationKeeper::checkUpkeep and LiquidationKeeper::performUpkeep due to encoding mistake

Summary

Incompatibility between LiquidationKeeper::checkUpkeep and LiquidationKeeper::performUpkeep due to an encoding mistake

Vulnerability Details

The LiquidationKeeper::checkUpkeep and LiquidationKeeper::performUpkeep are two key functions utilized by the LiquidationKeeper to carry out liquidations across various markets.
The issue is with how the performData used by LiquidationKeeper::performUpkeep is encoded by LiquidationKeeper::checkUpkeep. The function encodes the performData differently (with address(this) from what is expected to be decoded and used in performUpkeep.

The encoding:
Encodes a uint128 array with the address(this).
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol#L71-L87

function checkUpkeep(bytes calldata checkData)
external
view
returns (bool upkeepNeeded, bytes memory performData)
{
// ...
accountsToBeLiquidated = new uint128[](performLength);
// ...
bytes memory extraData = abi.encode(accountsToBeLiquidated, address(this));
return (upkeepNeeded, extraData);
}

The decoding:
Only expects only a uint128 array
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/external/chainlink/keepers/liquidation/LiquidationKeeper.sol#L101

function performUpkeep(bytes calldata peformData) external override onlyForwarder {
@> uint128[] memory accountsToBeLiquidated = abi.decode(peformData, (uint128[]));
LiquidationKeeperStorage storage self = _getLiquidationKeeperStorage();
(IPerpsEngine perpsEngine) = (self.perpsEngine);
​
perpsEngine.liquidateAccounts(accountsToBeLiquidated);
}

Impact

The LiquidationKeeper::performUpkeep won't work due to incompatibility issue with LiquidationKeeper::checkUpkeep.

Tools Used

Manual

Recommendations

Encode the performData correctly

AccountNFT is not upgradable because it inherits the wrong version of the Openzeppelin library

Summary

AccountNFT inherits the non-upgradable version of the Openzeppelin library, restricting its full upgradability with the PerpEngine.

Vulnerability Details

Zaros is an upgradeable smart contract system with functionalities for upgrading the different parts of the system. However, one key part of it that isn't upgradable is the AccountNFT contract. AccountNFT is an ERC721Enumerable contract that utilizes Openzeppelin. The problem is the contract inherits the non-upgradable version of the library, which is not the recommended approach to using the Oppenzeppelin library on upgradable systems.

Area of interest: https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/account-nft/AccountNFT.sol#L9

The GlobalConfigurationBranch::setTradingAccountToken function which enables the admin to update the address of the account token NFT contract, is a clear suggestion that the protocol might desire to upgrade the AccountNFT contract in the future. But using the setTradingAccountToken to make such upgrades, can expose the system to several risks, considering how intertwined with PerpEngine the contract is. For instance, there could be a mismatch of users and their trading accounts after such an upgrade.

/// @notice Sets the address of the account token NFT contract.
/// @param tradingAccountToken The account token address.
function setTradingAccountToken(address tradingAccountToken) external onlyOwner {
if (tradingAccountToken == address(0)) {
revert Errors.TradingAccountTokenNotDefined();
}
​
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
globalConfiguration.tradingAccountToken = tradingAccountToken;
​
emit LogSetTradingAccountToken(msg.sender, tradingAccountToken);
}

Impact

Inability to upgrade the AccountNFT contract with the rest of PerpEngine.

Tools Used

Manual

Recommendations

Inherit the upgradable version of Openzeppelin ERC721Enumerable.

False Liquidation of Trading Accounts Due to Undercounting of Margin Balance

Low

Summary

The protocol currently does not enforce USDz as a collateral token. This omission can result in inaccurate accounting of trading account margin balances.

Vulnerability Details

On Zaros, profitable traders receive USDz when their positions are settled, and this amount is added to their margin balance. This setup necessitates USDz as a required collateral token to accurately reflect trading account margin balances. However, the protocol fails to enforce this requirement, potentially leading to undercounted margin balances. Consequently, traders may face difficulties withdrawing their profits. The automatic issuance of PnL in USDz mandates support for USDz as a margin token, yet this requirement is not enforced in the code. This oversight increases the risk of compromising traders' positions due to unforeseen circumstances, potentially resulting in false liquidation of trading accounts.

// if trader's old position had positive pnl then credit that to the trader
if (ctx.pnlUsdX18.gt(SD59x18_ZERO)) {
ctx.marginToAddX18 = ctx.pnlUsdX18.intoUD60x18();
tradingAccount.deposit(ctx.usdToken, ctx.marginToAddX18);
​
// mint settlement tokens credited to trader; tokens are minted to
// address(this) since they have been credited to trader's deposited collateral
//
// NOTE: testnet only - this call will be updated once the Market Making Engine is finalized
LimitedMintingERC20(ctx.usdToken).mint(address(this), ctx.marginToAddX18.intoUint256());
}

Impact

Undercounting of trading account collateral balances can lead to false liquidation of accounts.

Tools Used

Manual

Recommendations

It is recommended to enforce the inclusion of USDz as an acceptable collateral type in the protocol's codebase. This change should be made mandatory and immutable to ensure accurate accounting of trading account margin balances and prevent potential false liquidations.

Weak Referral Code System Leading to Loss of Rewards for Affiliates

Summary

The current referral code system allows users to change their referral code each time they create a new account, leading to a potential loss of rewards for the original affiliate who onboarded the user.

Vulnerability Details

In the current system, a trader can specify a different referral code each time they create a new account. Since the protocol only recognizes one affiliate per user, if a user chooses a different affiliate when creating a new account, the original affiliate who did the initial work of onboarding the user misses out on the rewards that should have been attributed to them.

Impact

Loss of rewards for network affiliates, which can discourage their continued participation in the referral program.

Tools Used

Manual

Recommendations

Implement stricter rules for referral codes, such as binding the user to the original affiliate who onboarded them or limiting the ability to change referral codes across accounts.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Referrals should be set per trading account id instead of per trader

Support

FAQs

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