Aori
Smart Contract Security Assessment
May 8, 2023
![Aori](/audits/img/logos/aori.jpeg)
SUMMARY
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:
- 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:
- 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.
- 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
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]);
…
}
HIGH SEVERITY
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]];
…
} - 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.
Generally, neither the logic of the function itself nor that of its callers seems completely worked out. It seems that:
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.