Aori

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);
}

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));
}

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;
}

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]);
    	…
}

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.

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.

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());
…

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.

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);
    	}
}

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;
    	}
}

• 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.

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.

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

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);
  …
}

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;
}

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.

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.

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;
   	}
}

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”?

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];
}

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.

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

The field of the structure is never used.

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

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.

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

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);
    	}
 …
}

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.