Skip to main content
Dedaub

Aori

Smart Contract Security Assessment

May 8, 2023

Aori

SUMMARY

ID
Description
STATUS

ABSTRACT

Dedaub was commissioned to perform a security audit of the Aori Protocol’s Margin functionality, codenamed Aori Prime. The protocol aims to enable under-collateralized, decentralized options, as an add-on to the baseline Aori protocol, which handles the issuing and exchange of fully-collateralized options.

This audit report covers commit hash 7aae1773f1aec0f9fa0d34fc2fbad907b2100638 of the at-this-time private repository https://github.com/elstongun/Aori. The fixes were locally inspected (i.e., without a complete re-audit of the codebase) at commit hash a0e81eb0b56a3f7baf66d222dc6ba6af43f95a49. Especially files outside the audit scope (i.e., AORI.sol, AoriPut.sol, AoriCall.sol) have been covered less-than-completely, as a result.


SETTING & CAVEATS

Two auditors worked on the codebase for 4 days on the following contracts:

src/
  • Orderbook.sol
  • Margin/
    • MarginManager.sol
    • PositionRouter.sol
    • Structs.sol
    • Vault.sol

In understanding the new functionality, we briefly inspected files AORI.sol and AoriPut.sol/AoriCall.sol. Although these are outside the audit scope, several issues in the report refer to these files, as we have noticed them in passing.

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Although a high-level specification describing interactions within the protocol was provided, functional correctness (i.e. issues in "regular use") was a secondary consideration. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

In our opinion, the project is not ready for deployment and requires another design and implementation round. This is evident in:

  • The numerous critical and high severity issues identified;
  • The logical flaws in important parts of the codebase, including Orderbook::fillAsks/fillBids (which require complete rewriting to function correctly) and AORI::claimAccrual;
  • The lack of clarity and detailed explication of the financial model for margin (i.e., under-collateralized) options. There is no documentation of how funds change hands (e.g., how collateral deposited for an under-collateralized option becomes interest in the Vault and how this is proportionately reflected in losses/gains at option settlement time). Furthermore, there are clear flaws or significant questions in the financial model:
    • The code gives no suggestion as to how the keeper can liquidate an under-collateralized position: the code requires the keeper to hold the options to be burned, yet these have been handed over to the external requester of opening a short position.
    • Options are priced dynamically, yet the Vault functionality assigns them a value (for share-accounting purposes) equal to the Vault’s original investment in an option. This means that investors will decide to enter/leave a Vault based on the current value of the options it holds. This creates a situation where investors have more information than the Vault contracts and can exploit them.

The project’s test suite does not go into detail over any such complex scenarios. There are no tests of liquidations or even of filling asks/bids when there are multiple offers at a given price, as well as other offers at different prices in the system. Thorough testing and financial simulation is required before deployment in the hostile environment of the mainnet.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

C1

AORI::whitelistVault has no access control

C1CRITICAL

AORI::whitelistVault has no access control
resolved

The function has no checks over who is calling it, yet performs updates of important data, such as the oracle of a Vault (or merely the fact that an address is a Vault).

function whitelistVault(ERC4626 vault, address oracle) public {
vaultParams memory localParams;
localParams.whitelisted = true;
localParams.oracle = oracle;
vaults[vault] = localParams;
allVaults.push(vault);
}

C2

AORI::stakeLPTokens lacks access control

C2CRITICAL

AORI::stakeLPTokens lacks access control
resolved

The function is callable by anyone and allows passing in any ERC20 token as vault, which enables taking funds (which end up in AORI) from any user with approvals and locking them up for as long as the attacker wants (within given parameters).

function stakeLPTokens(ERC4626 vault, address receiver, uint256 shares,
uint256 lockUpTime) public {
require(shares >= 0 && vault.balanceOf(receiver) >= 0, "here");
if(lockUpTime == 2630000) {
boostMultiplier[receiver] == 1000;
} else if(lockUpTime == 7890000) {
boostMultiplier[receiver] == 1250;
} else if (lockUpTime == 15780000) {
boostMultiplier[receiver] == 1500;
} else if (lockUpTime == 31560000) {
boostMultiplier[receiver] == 1750;
} else if(lockUpTime == 604800) {
//for no lockup users
boostMultiplier[receiver] == 500;
} else {
revert("Incorrect lockup time");
}
userLockUpTime[receiver] = lockUpTime;
vault.transferFrom(receiver, address(this), shares);
// Dedaub: any token can be used as "vault" by an attacker
stakerBal[receiver][vault] = shares;
stakers.push(receiver);

emit Deposit(receiver, shares, address(vault));
}

C3

The logic of Orderbook::fillAsks and fillBids is flawed

C3CRITICAL

The logic of Orderbook::fillAsks and fillBids is flawed
resolved

The functions Orderbook::fillAsks and Orderbook::fillBids are not based on sound logic. There is a search for asks (resp. bids) in a global array, up to a certain filledAskIndex, which is not correctly maintained. The logic should return to the earlier approach of using the asksAtPrice/bidsAtPrice mappings, which are currently not correctly updated or read in the code. Finding the asks/bids at a given price through a global search is too inefficient to be possible anyway (see issue M1), even if the current filledAskIndex were correctly maintained.
(More comments in the code fragment below.)

// Dedaub: filledAskIndex doesn't serve its intended purpose, does not indicate
// the asks at the right price, the expression length - filledAskIndex does not
// have a reasonable meaning.
function fillAsks(address receiver, uint256 _USDCPerOPTION, uint256 USDCSize, uint256 seatId) external{
require(USDCSize > 0);
USDC.transferFrom(msg.sender, address(this), USDCSize);
uint256 amountToFill = USDCSize;
uint256 bal = asks[filledAskIndex[_USDCPerOPTION]].totalUSDCWanted();
// Dedaub: Unnecessary
if(bal == 0) {
filledAskIndex[_USDCPerOPTION++]; // Dedaub:! No-op !
}
for (uint256 i; i < asks.length - filledAskIndex[_USDCPerOPTION]; i++) {
bal = asks[filledAskIndex[_USDCPerOPTION]].totalUSDCWanted();
// Dedaub: totalUSDCWanted is not reliable! Can be inflated.
if(bal <= amountToFill) {
amountToFill -= bal;
USDC.approve(address(asks[filledAskIndex[_USDCPerOPTION]]), bal);
asks[filledAskIndex[_USDCPerOPTION]].fill(receiver, bal, seatId);
IERC20(address(USDC)).decreaseAllowance(
address(asks[filledAskIndex[_USDCPerOPTION]]),
USDC.allowance(address(this),
address(asks[filledAskIndex[_USDCPerOPTION]])));
filledAskIndex[_USDCPerOPTION]++;
}
}
totalVolume += USDCSize;
}

C4

MarginManager::settlePosition allows anyone to receive the collateral of a settled position

C4CRITICAL

MarginManager::settlePosition allows anyone to receive the collateral of a settled position
resolved

Due to the fact that no checks are performed on the account argument of the function, anyone who calls MarginManager::settlePosition may specify any address to receive a portion of the collateral that got used to open the position.

function settlePosition(address account, bytes32 key) public  {
accruePositionInterest(key);

Structs.doTransferOut(underlying, account, toTransferOut);
emit PositionRemoved(key, positions[key]);

}


HIGH SEVERITY

H1

AORI::unstakeLPTokens has no access control

H1HIGH

AORI::unstakeLPTokens has no access control
resolved

The function performs no checks against the caller, meaning that anyone can un-stake the tokens of another user.

function unstakeLPTokens(ERC4626 vault, address receiver, uint256 shares) public {
require(shares > 0 && stakerBal[receiver][vault] > 0, "No deposits");
require(block.timestamp >= userLockUpTime[receiver], "Cooldown has not been completed");

uint256 balPre = stakerBal[receiver][vault];
stakerBal[receiver][vault] = balPre - shares;
doTransferOut(vault, receiver, shares);
}

This causes the receiver to lose on any potential gains from a prolonged staking period.

H2

The logic of AORI::claimAccrual is flawed

H2HIGH

The logic of AORI::claimAccrual is flawed
n/a, code removed

There is no correlation between the ordering of the stakers and allVaults arrays. This means that using the i induction variable to access both arrays at the same time makes the iteration present inside the function problematic. The loop:

    • Does not iterate over all the actual stakers of a vault.

    • Will most likely revert in actual executions as the two arrays will have different lengths

      function claimAccrual() public {
      uint256 rewards;
      uint256 percentToBeMined = (block.timestamp - lastAccrueTime) * percentPerSecond;
      lastAccrueTime = block.timestamp;
      uint256 tokensMined = (percentToBeMined * expScale) / (maxLiqMining - liqMined);

      for(uint i; i < allVaults.length; i++) {
      if(stakerBal[stakers[i]][allVaults[i]] == 0){
      continue;
      } else {
      uint256 bal = stakerBal[stakers[i]][allVaults[i]];

      }

    Generally, neither the logic of the function itself nor that of its callers seems completely worked out. It seems that:

    • the function should claim rewards for a specific staker only, when the staker initiates the transaction that reaches the function;
    • the function should be called at every change of stake for the given staker, not just at the time of claimAORI.

H3

MarginManager::settlePosition uses wrongly scaled values

H3HIGH

MarginManager::settlePosition uses wrongly scaled values
resolved

When calculating collateralMinusLoss the function always decreases the collateral of the position by a value that has 6 decimals of precision. By definition, position.collateral has the same number of decimals as the underlying token for call options and the same number of decimals as the USDC token (i.e., 6) for the case of put options. This consequently means that the amount of tokens that end up being transferred is larger than it should have been

function settlePosition(address account, bytes32 key) public  {

if(AoriCall(position.option).getSettlementPrice() >
AoriCall(position.option).strikeInUSDC()) {

// Dedaub: assume collateral has 1eU precision
// Dedaub: has (1e6 * 1eU) / 1e18 precision
uint256 usdcValOfCollateral = Structs.mulDiv(
AoriCall(position.option).getSettlementPrice(), position.collateral,
10**AoriCall(position.option).decimals()
);

//Dedaub: has a precision of (1e6 * 1e6) / (1e6 * 1eU) * 1e18 = 1e6/1eU * 1e18
uint256 percentLoss =
Structs.mulDiv(AoriCall(position.option).getSettlementPrice() -
position.strikeInUSDC, 10**6, usdcValOfCollateral);

//Dedaub: has 1eU - 1e6/1eU * 1e18 * 1eU / 1e18 = 1eU - 1e6 precision
collateralMinusLoss = position.collateral - Structs.mulDiv(percentLoss, position.collateral, 10**AoriCall(position.option).decimals());

if(AoriPut(position.option).strikeInUSDC() >
AoriPut(position.option).getSettlementPrice()
) {

//Dedaub: has 1eU - 1e6 * 1e18 / 1e18 = 1eU - 1e6 precision
collateralMinusLoss = position.collateral -
Structs.mulDiv(position.strikeInUSDC -
AoriPut(position.option).getSettlementPrice(), position.optionSize,
10**AoriPut(position.option).decimals());

H4

PositionRouter::getInterestOwed can end up calculating an unbounded value of interest

H4HIGH

PositionRouter::getInterestOwed can end up calculating an unbounded value of interest
resolved

The function calculates the interest to be paid by multiplying the interest rate with the time interval after the last accrual took place.

function getInterestOwed(..., address option, ... , uint256 lastAccrueTime) public view returns (uint256) {
...

interestFactor = ((getBorrowRate(underlying) + entryMarginRate) / 2) * (block.timestamp - lastAccrueTime);
return Structs.mulDiv(interestFactor, optionSize, 10**18);
}

Since PositionRouter::getInterestOwed is called in MarginManager::accruePositionInterest which in turn is called in MarginManager::settlePosition, the block.timestamp value in the above calculation can be arbitrarily past the maturity date.

This could not only cause a situation where users are paying more interest than they should, but also raises considerations on whether calls to MarginManager::settlePosition can end up reverting because of insufficient balance in the MarginManager contract for the interval that interest is calculated. In principle, the interval is unbounded and cannot be accounted for beforehand.

H5

Wrong usage of PositionRouter::getInterestOwed

H5HIGH

Wrong usage of PositionRouter::getInterestOwed
resolved

This function is called only in MarginManager::accruePositionInterest, and it is always called with the isCall argument set to false

function accruePositionInterest(bytes32 key) public returns (bool) {

interestOwed = positionRouter.getInterestOwed(false, underlying, position.option,);

This causes the interest calculation for call options to be incorrect:

function getInterestOwed(bool isCall,, uint256 strikeInUSDC, uint256 optionSize, ) public view returns (uint256) {

if(isCall) {

interestFactor =
return Structs.mulDiv(interestFactor, optionSize, 10**18);
} else {

interestFactor =
return Structs.mulDiv(interestFactor, strikeInUSDC, 10**18);
}
}

H6

Flawed logic behind how PositionRouter::getInterestOwed calculates the interest of put options

H6HIGH

Flawed logic behind how PositionRouter::getInterestOwed calculates the interest of put options
resolved

For put options, the function calculates the interest to be paid as a percentage of the strike price

function getInterestOwed(bool isCall, ..., uint256 strikeInUSDC, ... ) public view returns (uint256) {

if(isCall) {

interestFactor = ((getBorrowRate(underlying) + entryMarginRate) / 2) * (block.timestamp - lastAccrueTime);
return Structs.mulDiv(interestFactor, optionSize, 10**18); //1e6 * 1e6 / 1e6
} else {

interestFactor = ((getBorrowRate(underlying) + entryMarginRate) / 2) * (block.timestamp - lastAccrueTime);
return Structs.mulDiv(interestFactor, strikeInUSDC, 10**18);
}
}

However, this is not the amount of USDC that the Vault will have used to mint the Put option when Vault::mintOptions is called

function mintOptions(uint256 amountOfUnderlying, address option, uint256 seatId, address account, bool isCall) public returns (uint256) {

totalBorrows += amountOfUnderlying;
uint256 optionsMinted;
if(isCall) {

} else {
AoriPut(option).USDC().approve(option, amountOfUnderlying);
optionsMinted = AoriPut(option).mintPut(amountOfUnderlying, account, seatId);
AoriPut(option).getOptionsSold(address(this));
return optionsMinted;
}
}

H7

Inherited functionality in Vault should be overridden

H7HIGH

Inherited functionality in Vault should be overridden
resolved
    • The Vault contract inherits from OpenZeppelin’s ERC-4626 implementation contract. This means that functions like ERC6426::redeem, ERC6426::withdraw, ERC6426::deposit and ERC6426::mint are also inherited as public functions by default.

      It is advised that those functions get overridden in order to avoid potential accounting issues in the Vault contract. Namely, anyone calling those functions can deposit/withdraw assets and bypass the checks that functions like Vault::depositAssets, Vault::withdrawAssets and Vault::mintOptions put in place.
      • Another thing that should be noted is that the default ERC4626 implementation is vulnerable to an inflation attack on the internal price of the shares. An attacker can perform such an attack by depositing to a (nearly) empty vault, and continuously front-run other users’ deposits by performing direct transfers to the vault.

        The version of the OpenZeppelin implementation that is currently used does not allow the front-runner of an empty vault to profit out of users’ deposits. This is achieved by using a virtual offset in the precision of the shares and the underlying token (ERC4626::_decimalsOffset) in combination with virtual shares and assets during calculations.

        Even the default value of ERC4626::_decimalsOffset (0) leaves no room for the attacker to profit from front-running deposits. However, users can still potentially get griefed and receive 0 shares if the front-runner donates an amount analogous to a victim’s deposit. We recommend that the project has a look at OpenZeppelin’s documentation [1,2] for this issue.


MEDIUM SEVERITY

M1

Arrays can become too large to iterate

M1MEDIUM

Arrays can become too large to iterate
resolved

In several cases, arrays can become too large to iterate over. Any on-chain array should hold at most a few tens of elements. At over a hundred elements, the gas cost of iterating over the array contents becomes prohibitive. At just a few hundred elements, the block gas limit can be reached and no transaction can ever iterate over an array, resulting in functionality lockup.

Therefore, the smart contract logic should avoid global arrays, such as:

AORI::allVaults:

function whitelistVault(ERC4626 vault, address oracle) public {

allVaults.push(vault); // Dedaub: can grow without bound
}


Orderbook::asks (which should also be protected from insertion of asks with _OPTIONSize zero):

function createAsk(uint256 _USDCPerOPTION, uint256 _OPTIONSize) public
returns (Ask) {

asks.push(ask);

}


AORI::stakers:

function stakeLPTokens(ERC4626 vault, address receiver, uint256 shares,
uint256 lockUpTime) public {

stakers.push(receiver);

}


Generally, arrays that get iterated over (e.g., with a for loop) should be avoided in smart contracts. If it is certain that the arrays cannot grow that big in normal use, there should be at least a limit in the code so that malicious actors cannot grow them for the purpose of griefing the contract.

M2

Counter-intuitive fee distribution in MarginManager::accruePositionInterest

M2MEDIUM

Counter-intuitive fee distribution in MarginManager::accruePositionInterest
resolved

While the owner of the MarginManager is the one receiving most of the accrued interest, the Vault which has minted the position receives only a percentage of it.

function accruePositionInterest(bytes32 key) public returns (bool) {

uint256 fee = Structs.mulDiv(interestOwed, (positionRouter.seats()).marginFee(), 10000);

Structs.doTransferOut(underlying, owner(), interestOwed - fee);
Structs.doTransferOut(underlying, address(lpTokens[underlying]), fee);

}

It would make more sense for the Vault to receive most of the interest, since the collateral of a margin position is coming out of it

M4

Interest is incurred for a time period when collateral is not in the position

M4MEDIUM

Interest is incurred for a time period when collateral is not in the position
resolved

The code of MarginManager::addCollateral first adds the extra collateral to a position and then calls accruePositionInterest. This has the effect that the new collateral incurs interest charges even for the period since the last accrual (and until now), although the collateral was added just now.

function addCollateral(bytes32 key, uint256 collateralToAdd) public
returns (uint256) {

underlying.transferFrom(msg.sender, address(this), collateralToAdd);
position.collateral = currentCollat + collateralToAdd;
accruePositionInterest(key);

}

M5

It is counter-intuitive that interest paid counts as a repayment of the borrowed sum

M5MEDIUM

It is counter-intuitive that interest paid counts as a repayment of the borrowed sum
resolved

In MarginManager::accruePositionInterest, the amount of interest owed counts as a repayment in the Vault accounting. This does not seem valid, since the Vault uses the quantity that’s getting reduced (totalBorrows) to count its exposure in issued options, and this exposure does not change by the charging of interest.

// MarginManager
function accruePositionInterest(bytes32 key) public returns (bool) {

lpTokens[underlying].repaid(interestOwed);

}

// Vault
function repaid(uint256 assets) public returns (uint256) {
require(msg.sender == address(manager));
totalBorrows -= assets;
return assets;
}


LOW SEVERITY

L1

Persistent misunderstanding regarding Solidity types and function dispatch

L1LOW

Persistent misunderstanding regarding Solidity types and function dispatch
largely resolved

Largely resolved

4th bullet below is still open

Throughout the code base there are misunderstandings regarding the use of types in Solidity and how functions are dispatched. These lead to much duplicated code.

For instance (Orderbook::cancelOrder):

if(_isAsk) {
require(msg.sender == Ask(order).maker());
Ask(order).cancel();
} else {
require(msg.sender == Bid(order).maker());
Bid(order).cancel();
}

The code in the “then” and “else” branch of the conditional is identical. The functions called have the same signature and they only superficially differ because of the types used to convince the compiler that the call is valid. But neither the compiler’s checks nor the produced code have the slightest difference in the two paths. Replacing the “if” with just the body of the “then” branch would have exactly the same effect.

A cleaner solution would be to create a super-interface IAsk and IBid (and similarly for ICall and IPut) that asks/bids (resp. calls/puts) both implement and which contains the common functions. In this way, several if/else branches in the code will be unified, resulting in significantly more concise code.

Other instances of the above pattern include:

  • in PositionRouter::openShortPositionRequest, the “then” and “else” branches are largely duplicates (although not entirely, due to the first two lines);

  • in PositionRouter::getInterestOwed, the “then” and “else” branches are largely duplicates;

  • in PositionRouter::isLiquidatable, the “then” and “else” branches of the first conditional are the exact same code;

  • in Vault::settleITMOption, two sides of an || are entirely equivalent:

    require(AoriCall(option).endingTime() <= block.timestamp ||
    AoriPut(option).endingTime() <= block.timestamp,
    "Option has not expired");
  • in the same function, two sides of an “if” are equivalent (and some code is unused):

    if(isCall) {
    vars.optionsSold = AoriCall(option).getOptionsSold(address(this));
    // Dedaub: unused
    AoriCall(option).sellerSettlementITM();
    isSettled[option] = true;
    } else {
    vars.optionsSold = AoriPut(option).getOptionsSold(address(this));
    AoriPut(option).sellerSettlementITM();
    isSettled[option] = true;
    }
  • similarly in Vault::settleOTMOption.

L2

Persistent misunderstanding regarding the semantics of ERC20 token operations

L2LOW

Persistent misunderstanding regarding the semantics of ERC20 token operations
partly resolved

Partly resolved

not in AoriPut/AoriCall/PositionRouter

Throughout the code there are several instances of ERC20 approvals given and then revoked, with a transfer operation performed in the middle. ERC20 approvals are only necessary (or useful) when the recipient of the approval performs a transferFrom operation. There is no impact on a direct transfer. There should be no approvals given (or decreased) to a receiver of funds in all the cases below.

For instance, in AORI.sol:

function doTransferOut(ERC20 token, address receiver, uint256 amount) internal {
token.transfer(receiver, amount);
// Dedaub: the rest is unnecessary
if(token.allowance(address(this), msg.sender) > 0) {
token.decreaseAllowance(receiver,
token.allowance(address(this), receiver));
}
}

In AoriCall::liquidationSettlement:

UNDERLYING.approve(msg.sender, UNDERLYINGToReceive); // Dedaub:unnecessary
UNDERLYING.transfer(msg.sender, UNDERLYINGToReceive);
if(USDC.allowance(address(this), msg.sender) > 0) { // Dedaub:unnecessary
UNDERLYING.decreaseAllowance(manager,
UNDERLYING.allowance(address(this), msg.sender));
}

Similar code is found in:

  • Orderbook::doTransferOut (with the function being unused anyway);
  • PositionRouter::executeOpenPosition (twice);
  • Structs::doTransferOut.

L3

Return value of MarginManager::accruePositionInterest is invalid

L3LOW

Return value of MarginManager::accruePositionInterest is invalid
resolved

The return value of that function does not return useful information: it compares interestOwed to position.collateral, but the latter has already been reduced by the former.

// MarginManager
function accruePositionInterest(bytes32 key) public returns (bool) {

uint256 currentCollat = position.collateral;

position.collateral = currentCollat - interestOwed;

if(position.collateral > interestOwed) {
return true;
} else {
return false;
}
}

L4

The role of tick sizing is unclear

L4LOW

The role of tick sizing is unclear
resolved

In Orderbook::createAsk/createBid, there are adkMinTick/bidMinTick quantities, but their check in the code has unclear utility.

require(askMinTick % _USDCPerOPTION == askMinTick,
"Prices must match appropriate tick size");

This check is equivalent to “askMinTick <= _USDCPerOPTION” (given that askMinTick is > 0). Was the intent perhaps to have “_USDCPerOPTION % askMinTick == 0”?

L5

Return value of MarginManager::liquidatePosition is invalid

L5LOW

Return value of MarginManager::liquidatePosition is invalid
resolved

The return value (and emitted event) of that function is always zero, since the entry gets deleted right above.

function liquidatePosition(bytes32 key, uint256 fairValueOfOption,
address liquidator) public returns (Structs.Position memory) {

delete positions[key];
emit PositionRemoved(key, positions[key]);
return positions[key];
}

L6

The check for the cooldown of the caller in Vault::withdrawAssets seems to serve no purpose

L6LOW

The check for the cooldown of the caller in Vault::withdrawAssets seems to serve no purpose
open

The check for the cooldown of the caller does not add any layers of security

function withdrawAssets(uint256 assets, address receiver) public {
require(block.timestamp >= cooldownTime[msg.sender], "Cooldown has not passed");
IAORI(aori).unstakeLPTokens(Vault(address(this)), receiver, convertToShares(assets));
withdraw(assets, receiver, receiver);
}

An account can specify the receiver of the shares when calling Vault::depositAssets. So in reality, checks regarding cooldown should only take place for the account that shares belong to. AORI::stakeLPTokens, AORI::unstakeLPTokens properly account for the lock up time for the specified receiver.



CENTRALIZATION ISSUES

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

N1

Some entities are considered trusted

N1CENTRALIZATION

Some entities are considered trusted
info

The protocol assumes the existence of keepers who judge the value of investments from the margin Vaults and make decisions over the Vault funds.



OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

AORI::struct vaultParams::vault is unused

A1ADVISORY

AORI::struct vaultParams::vault is unused
info

The field of the structure is never used.

struct vaultParams {
address vault; // Dedaub: unused
address oracle;
bool whitelisted;
}

A2

In MarginManager (multiple functions) positions are not checked to exist

A2ADVISORY

In MarginManager (multiple functions) positions are not checked to exist
info

Several functions in MarginManager (e.g., settlePosition, addCollateral) look up a position by key in the positions mapping but do not check that the result exists. This seems to be currently fine in the code because a function (e.g., USDC()) is called on the resulting position.option, therefore the Solidity compiler will ensure that the position.option is a contract address. However, it is generally a bad practice and it is easy to accidentally produce code that accepts a zero value without reverting.

A3

Unused fields/variables, no-op code

A3ADVISORY

Unused fields/variables, no-op code
info

Some variables/calls are unused/unnecessary:

  • PositionRouter::lastPosition (which would be unlikely to be useful anyway);

  • isCall in PositionRouter::getInitialMargin, with the entire function also not used in the contracts but perhaps of external use;

  • Vault::USDCScale

  • In Vault::mintOptions:

      AoriPut(option).getOptionsSold(address(this));  // Dedaub: no-op
  • The local variable decimalDiff in PositionRouter::getInterestOwed is unused

  • The local variable vars.optionsSold in Vault::settleITMOption is unused

A4

The intent behind the exact value of some constants is unclear

A4ADVISORY

The intent behind the exact value of some constants is unclear
info

It is not clear why lockupTime in Vault::depositAssets is in subdivisions of 31560000, since this is neither 365 * 24 * 3600, nor 365.25 * 24 * 3600.

Also PositionRouter::liquidationRatio is claimed (in a comment) to have a default of 20000, which seems excessively high: based on the calculation in the code, it would require positions to be 200% collateralized.

uint256 public immutable BPS_DIVISOR = 10000;

uint256 public liquidationRatio; //in bps over 10000, default 20000

function isLiquidatable(...) {

uint256 liquidationThreshold =
Structs.mulDiv(positionVal, liquidationRatio, BPS_DIVISOR);
if(liquidationThreshold >= collateralVal) {
return (collateralVal, positionVal, true);
}

}

A5

Compiler bugs

A5ADVISORY

Compiler bugs
info

The code has the compile pragmas ^0.8.18. For deployment, we recommend no floating pragmas, i.e., a specific version, so as to be confident about the baseline guarantees offered by the compiler. Version 0.8.18, in particular, has no known bugs, currently.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub