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

When SettlementBranch is updated there is no way to change the owner of the USDz

SBSecurity

Summary

SettlementBranch lacks functionality to transfer the USDToken ownership and this will block the Zaros team from performing a replacement because when updated the owner of the USDToken will remain old SettlementBranch and positions with +ve PnL will not be able to be modified because USDToken::mint has onlyOwner modifier:

function mint(address to, uint256 amount) external onlyOwner {
_requireAmountNotZero(amount);
_mint(to, amount);
}

SettlementBranch.sol

function _fillOrder(
uint128 tradingAccountId,
uint128 marketId,
uint128 settlementConfigurationId,
SD59x18 sizeDeltaX18,
UD60x18 fillPriceX18
)
internal
virtual
{
...MORE CODE
// 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());
}
}

Vulnerability Details

The idea of the Zaros team is to mint additional collateral in form of their USD token when position is in profit but this will not be possible when RootUpgrade::replaceBranch is called for replacing the Settlement:

RootUpgrade.sol

function replaceBranch(Data storage self, address branch, bytes4[] memory selectors) internal {
// slither-disable-next-line unused-return
self.branches.add(branch);
​
uint256 cachedSelectorsLength = selectors.length;
​
for (uint256 i; i < cachedSelectorsLength; i++) {
bytes4 selector = selectors[i];
address oldBranch = self.selectorToBranch[selector];
​
if (selector == bytes4(0)) {
revert Errors.SelectorIsZero();
}
if (oldBranch == address(this)) {
revert Errors.ImmutableBranch();
}
if (oldBranch == branch) {
revert Errors.FunctionFromSameBranch(selector);
}
if (oldBranch == address(0)) {
revert Errors.NonExistingFunction(selector);
}
​
// overwrite selector to new branch
self.selectorToBranch[selector] = branch;
​
// slither-disable-next-line unused-return
self.branchSelectors[branch].add(selector);
​
// slither-disable-next-line unused-return
self.branchSelectors[oldBranch].remove(selector);
​
// if no more selectors, remove old branch address
if (self.branchSelectors[oldBranch].length() == 0) {
// slither-disable-next-line unused-return
self.branches.remove(oldBranch);
}
}
}

Owner remains the old contract and this prevents positions in profit to be modified.

As we can see this function only prevent from replacing the RootProxy and no other branches are specified, which means actions above are possible. Although there is comment stating that this is only for testnet the scope of this audit will represent the initial version of the protocol and that means we should accept the code will be deployed as is.

Impact

Upgrading the SettlementBranch is not possible as it will block important functionality for positions in profit and the only way they to be close is to have -ve PnL (which is loss of funds for the trader) or to be liquidated unfairly.

Tools Used

Manual Review

Recommendations

Add transferOwnership function in SettlementBranch.

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.