Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Whitelisted users can create trading accounts for non-whitelisted users

elolpuer

Offtop

I know that TradingAccountBranch.sol is out of scope, but after the first audit, devs added a whitelist system that is used in TradingAccountBranch.sol and is in scope—Whitelist.sol.

Summary

The protocol's whitelist system is designed to provide access to protocol only to whitelisted users by restricting the creation of trading accounts. However, whitelisted users can create an unlimited number of trading accounts and transfer them to others, bypassing the restrictions imposed on users.

Vulnerability Details

In the TradingAccountBranch.sol:createTradingAccount() if whitelist enabled, only users in list can create trading accounts:

function createTradingAccount(
bytes memory referralCode,
bool isCustomReferralCode
)
public
virtual
returns (uint128 tradingAccountId)
{
// fetch storage slot for perps engine configuration
PerpsEngineConfiguration.Data storage perpsEngineConfiguration = PerpsEngineConfiguration.load();
​
// enforce whitelist if enabled
address whitelistCache = perpsEngineConfiguration.whitelist;
if (whitelistCache != address(0)) {
if (!Whitelist(whitelistCache).verifyIfUserIsAllowed(msg.sender)) {
revert Errors.UserIsNotAllowed(msg.sender);
}
}
...
}

But there's no limit to the number of accounts you can create, nor is there a limit to transfering them if whitelist is still enabled.

//@AUDIT - NO CHECKS IF WHITELIST IS STILL ENABLED AND USER IN IT
/// @notice Used by the Account NFT contract to notify an account transfer.
/// @dev Can only be called by the Account NFT contract.
/// @dev It updates the Trading Account stored access control data and cancel existing pending orders.
/// @param to The recipient of the account transfer.
/// @param tradingAccountId The trading account id.
function notifyAccountTransfer(address to, uint128 tradingAccountId) external {
_onlyTradingAccountToken();
​
// load trader's orders
MarketOrder.Data storage marketOrder = MarketOrder.load(tradingAccountId);
​
// cancel pending order if it exists
if (marketOrder.marketId != 0) {
// reverts if a trader has a pending order and that pending order hasn't
// existed for the minimum order lifetime; pending orders can't be cancelled
// until they have existed for the minimum order lifetime
marketOrder.checkPendingOrder();
// cancel pending order
marketOrder.clear();
​
emit OrderBranch.LogCancelMarketOrder(msg.sender, tradingAccountId);
}
​
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
tradingAccount.owner = to;
}

PoC

Add this test to test/integration/perpetuals/trading-account-branch/createTradingAccount/createTradingAccount.t.sol:

function test_WhitelistUserCanGiveAccountsToEveryone() external givenTheTradingAccountTokenIsSet {
changePrank(users.owner.account);
//enable whitelist
configureSystemParameters();
​
changePrank(users.naruto.account);
//create new accounts that not in whitelist
address[5] memory usersNotInWhitelist;
usersNotInWhitelist[0] = address(0x111111);
usersNotInWhitelist[1] = address(0x222222);
usersNotInWhitelist[2] = address(0x333333);
usersNotInWhitelist[3] = address(0x444444);
usersNotInWhitelist[4] = address(0x555555);
for (uint256 i = 0; i < usersNotInWhitelist.length; i++) {
//verify caller is whitelist
assertEq(whitelist.verifyIfUserIsAllowed(users.naruto.account), true);
//verify user is not whitelist
assertEq(whitelist.verifyIfUserIsAllowed(usersNotInWhitelist[i]), false);
uint128 tradingAccountId = perpsEngine.createTradingAccount(bytes(""), false);
//verify trading account owner is caller
assertEq(tradingAccountToken.ownerOf(tradingAccountId), users.naruto.account);
tradingAccountToken.transferFrom(users.naruto.account, usersNotInWhitelist[i], tradingAccountId);
//verify trading account owner is now not whitelisted user
assertEq(tradingAccountToken.ownerOf(tradingAccountId), usersNotInWhitelist[i]);
//non-whitelisted users now have access to protocol
}
}

In cmd run this command:

forge test --mt test_WhitelistUserCanGiveAccountsToEveryone

Output:

Ran 1 test for test/integration/perpetuals/trading-account-branch/createTradingAccount/createTradingAccount.t.sol:CreateTradingAccount_Integration_Test
[PASS] test_WhitelistUserCanGiveAccountsToEveryone() (gas: 1054944)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 36.88ms (927.41µs CPU time)

Impact

Non-whitelisted users can access the protocol

Tools Used

Manual Review

Recommendations

Update TradingAccountBranch.sol:notifyAccountTransfer() like this:

function notifyAccountTransfer(address to, uint128 tradingAccountId) external {
_onlyTradingAccountToken();
​
+ // fetch storage slot for perps engine configuration
+ PerpsEngineConfiguration.Data storage perpsEngineConfiguration = PerpsEngineConfiguration.load();
+ // enforce whitelist if enabled
+ address whitelistCache = perpsEngineConfiguration.whitelist;
+ if (whitelistCache != address(0)) {
+ if (!Whitelist(whitelistCache).verifyIfUserIsAllowed(to)) {
+ revert Errors.UserIsNotAllowed(to);
+ }
+ }
​
// load trader's orders
MarketOrder.Data storage marketOrder = MarketOrder.load(tradingAccountId);
​
// cancel pending order if it exists
if (marketOrder.marketId != 0) {
// reverts if a trader has a pending order and that pending order hasn't
// existed for the minimum order lifetime; pending orders can't be cancelled
// until they have existed for the minimum order lifetime
marketOrder.checkPendingOrder();
// cancel pending order
marketOrder.clear();
​
emit OrderBranch.LogCancelMarketOrder(msg.sender, tradingAccountId);
}
​
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
tradingAccount.owner = to;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Whitelisted users can create trading accounts for non-whitelisted users

Support

FAQs

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