Olas - peanuts's results

[L-01] MIN_VESTING should be adjustable in Depository.sol

MIN_VESTING is currently set as 1 days in Depository.sol.

    uint256 public constant MIN_VESTING = 1 days;

This value cannot be changed, but ideally it should be flexible because it will give the protocol more opportunities to grow their system. For example, setting min vesting to less than a day can incentivise more people to bond their LP tokens for OLAS tokens. Changing the amount will incur more centralization risks, but since the protocol is heavily reliant on centralization already, it will be better to add more convenience for a slight increase in centralization risk.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Depository.sol#L209-L212

[L-02] getBonds() in Depository.sol may face out of gas error since bondCounter only increases for every bond created through deposit()

In getBonds(), the function attempts to get all the bonds from one account using a loop.

  uint256 numBonds = bondCounter;
        bool[] memory positions = new bool[](numBonds);
        // Record the bond number if it belongs to the account address and was not yet redeemed
        for (uint256 i = 0; i < numBonds; ++i) {

bondCounter increases everytime a user calls deposit() and will never decrease

 // Create and add a new bond, update the bond counter
        bondId = bondCounter;
        mapUserBonds[bondId] = Bond(msg.sender, uint96(payout), uint32(maturity), uint32(productId));
        bondCounter = uint32(bondId + 1);

This means that as more users start to call deposit() to create a bond, the value of bondCounter will increase. If it gets too large, the getBonds() function will run out of gas as it attempts to loop through every bond created just to find the bond that a particular account has created.

To give specific numbers, if Alice creates the first bond, bondCounter = 1, and there are 10000 bonds created after that, and Alice creates another bond bondCounter = 10001, then the function has to loop 10001 times just to find that Alice has created the first bond and the latest bond.

This function is extremely gas intensive. A better way is to create a mapping for the particular user every time a bond is created through deposit(), and push the new value into the mapping.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Depository.sol#L435-L447

[L-03] Users using the deposit() function in Depository.sol can be DoS'd by ensuring that payout > supply everytime

In deposit(), a user deposits a certain amount of LP tokens to get a certain amount of OLAS tokens. This calculation is done through IGenericBondCalculator(bondCalculator).calculatePayoutOLAS, and if the payout is greater than the supply, revert the whole function

// Check for the sufficient supply
        if (payout > supply) {
            revert ProductSupplyLow(token, productId, payout, supply);
        }

To calculate the exact amount such that payout == supply, the user has to do his own calculation and call calculatePayoutOLAS() directly.

  function calculatePayoutOLAS(uint256 tokenAmount, uint256 priceLP) external view
        returns (uint256 amountOLAS)
    {

Let's say that the remaining supply of OLAS token is 10000, and it takes 100 LP tokens to get 1000 OLAS tokens. Alice calls deposit() and deposit 100 LP tokens, with the intention of getting the last 1000 OLAS tokens. Another user sees this, and frontruns the transaction by depositing 1 wei of LP tokens and get 10 wei of OLAS tokens. Alice transaction then gets executed, but reverts because the payout is now greater than the supply. Alice has to calculate again and deposit 100-1wei of LP tokens to get 1000-10wei of OLAS tokens.

Instead of reverting when the payout > supply, the function should recalculate the payout so that payout == supply

   if (payout > supply) {
            // Have another calculation in here such that payout == supply
            // This way, users who want to deposit a large amount of LP tokens to get the remaining OLAS token supply will not get DoS'd easily
            // Another way is to have a minimum LP deposit, so users cannot DoS with 1 wei, but this will incur dust tokens
            // Dust tokens isn't an issue as the bond can be closed with the remaining supply returning back to the effectiveBond in Tokenomics contract
        }

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Depository.sol#L319-L326

[L-04] payout can still be non-zero when depositing LP tokens in Depository.sol

In deposit(), the comments state that payout cannot be zero since the price LP is non-zero

   // Calculate the payout in OLAS tokens based on the LP pair with the discount factor (DF) calculation
        // Note that payout cannot be zero since the price LP is non-zero, otherwise the product would not be created
        payout = IGenericBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP);

This is the calculation:

        uint256 totalTokenValue = mulDiv(priceLP, tokenAmount, 1);
        // Check for the cumulative LP tokens value limit
        if (totalTokenValue > type(uint192).max) {
            revert Overflow(totalTokenValue, type(uint192).max);
        }
        // Amount with the discount factor is IDF * priceLP * tokenAmount / 1e36
        // At this point of time IDF is bound by the max of uint64, and totalTokenValue is no bigger than the max of uint192
        amountOLAS = ITokenomics(tokenomics).getLastIDF() * totalTokenValue / 1e36;

If the price of the LP is below 1e18, and the tokenAmount is extremely small (in wei), the payout can be truncated to zero assuming that getLastIDF() returns 1e18.

totalTokenValue = 0.9e17 * 1 / 1 = 0.9e17

amountOLAS = 1e18 * 0.9e17 / 1e36 = 0

It is possible for the payout to return a non-zero number.

Check that payout is not zero before continuing the function.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/Depository.sol#L320C77-L320C88

[L-05] veOLAS allows for contracts to vote, but not veCRV

In veCRV, there is a check when creating a lock,

    self.assert_not_contract(msg.sender)

which means that smart contracts are not allowed to vote.

@internal
def assert_not_contract(addr: address):
    """
    @notice Check if the call is from a whitelisted smart contract, revert if not
    @param addr Address to be checked
    """
    if addr != tx.origin:
        checker: address = self.smart_wallet_checker
        if checker != ZERO_ADDRESS:
            if SmartWalletChecker(checker).check(addr):
                return
        raise "Smart contract depositors not allowed"

There is no such check in veOLAS.sol.

One possible reason to prevent smart contracts from participating is to reduce the attack vectors such as flashloan attacks.

To improve consistency of fork code, check for the contract existence size in veOLAS.sol as well.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/veOLAS.sol#L425-L434

[L-06] Users can create lock for others in veOLAS.sol, which opens up griefing capabilities

Users can call the createLockFor() function to create a lock for another account.

function createLockFor(address account, uint256 amount, uint256 unlockTime) external {
        // Check if the account address is zero
        if (account == address(0)) {
            revert ZeroAddress();
        }

        _createLockFor(account, amount, unlockTime);
    }

Griefers can create a lock for random users with 1 wei as the amount and 4 years as the unlock time to purposely grief users. Some genuine users of the OLAS protocol may not want to lock their tokens for 4 years, but a malicious griefer can frontrun their createLock function and create a lock for them with 4 years as the unlockTime. Since the unlockTime cannot be reduced after creation, the genuine users will be affected and have to create another account just to create a lock.

In the original veCRV implementation, there is no function that allows a user to create a lock for other users.

@external
@nonreentrant('lock')
def create_lock(_value: uint256, _unlock_time: uint256):
    """
    @notice Deposit `_value` tokens for `msg.sender` and lock until `_unlock_time`
    @param _value Amount to deposit
    @param _unlock_time Epoch time when tokens unlock, rounded down to whole weeks
    """

The only functionality in the veCRV implementation that allows user to help other users is the depositfor function.

It is best to remove the createLockFor function in veOLAS.sol to prevent any griefing capabilities. Just allow the msg.sender to create a lock for himself.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/veOLAS.sol#L411-L418

Summary and Overview

OLAS protocol has four parts, bonding, registries and voting and bridging. Users will find themselves integrating with voting and bonding more than other mechanisms.

For voting, the mechanism is similar to veCRV voting.

• Firstly, users have to acquire some OLAS tokens
• Users can create a lock and deposit OLAs tokens in exchange for voting power
• Voting power depends on the amount of OLAS tokens locked and the duration of the lock

For Bonding, users will deposit LP tokens (OLAS-X, where X can be another common token like ETH or DAI at the protocol's discretion) to get OLAS token. Owner will specify the bonding rubrics.

• The owner will enable a certain LP token for bonding
• Users will acquire the LP token (eg OLAS-ETH LP token), and deposit the LP token into the treasury contract
• The protocol function will calculate the conversion rate of the LP token to OLAS.
• Users will need to wait for MIN_VESTING period (1 days) for their bond to mature.
• Once the bond matures, users can withdraw their OLAS tokens from the Depository contract

Codebase quality analysis and Mechanism review

veOLAS.sol

Mechanism Review and Recommendation

• User can create a lock to deposit OLAS. User also specifies the duration of the lock.

• More OLAS deposited, more voting power. The longer the lock duration, the more voting power

• Users can create a lock for others and also increase the amount deposited for others

• Only the owners of the lock can increase the duration of the lock. Lock duration has a max limit

• This contract is extremely similar to the veCRV contract, so I did a comparison between both contracts

• veCRV does not allow contracts to create locks, but veOLAS does

• veCRV uses block epoch, but veOLAS uses block.number

• Recommend restricting contracts to have voting power, only addresses, to prevent any potential griefing/attacks

• Recommend not to allow users to create lock for other users to prevent griefing attacks (locking 1 wei for 4 years for an account)

Treasury.sol

Mechanism Review and Recommendation

• There are two types of ETH that is tracked, ETHOwned and ETHFromServices

• ETHOwned is withdrawn through withdraw() and ETHFromServices is withdrawn through withdrawToAccount()

• There is mapEnabledTokens[token] and mapTokenReserves[token]. Both points to the LP token. One of them is a boolean, one is the amount of reserves

• mapTokenReserves[token] is increased through depositTokenForOLAS() and decreased through withdraw()

• OLAS is minted through depositTokenForOLAS() and potentially withdrawToAccount()

• Recommend creating a modifier for msg.sender == owner since it is repeated many times in the code

• changeOwner() should have a two-step transfer

• changeManagers() should have a two-step transfer

• changeMinAcceptedETH() should have a upper limit (eg max minEth accepted is 0.5 ETH)

• pause and unpause mechanism only used for 2 functions, consider using for more functions

Depository.sol

Mechanism Review and Recommendation

• MIN_VESTING is one day, cannot be changed

• productId refers to the bond, and the productId is monotonically increasing

• productCounter will never decrease, only increase

• create() refers to how much OLAS can be supplied for a given LP amount

• every time create() is called, a new Product struct is created and saved in the mapBondProducts setting

• For the Product struct, PriceLP refers to the price of the LP, vesting refers to the number of seconds (not end time), token refers to the LP token address, supply refers to the OLAs token amount

• In deposit(), payout is calculated with amount and priceLP using the calculatePayoutOLAS function, supply is decreased, mapUserBonds is created, LP token is deposited, Olas token is minted

• In deposit(), mapBondProducts is auto deleted if supply reaches 0.

• getBonds(), if set to true, then will get only matured bonds from an account

• getBonds(), if set to false, then will get all bonds from an account.

• getBonds() loops all bonds, may cause out of gas error

• Recommend making MIN_VESTING flexible, changed with owner's discretion

• Checking whether msg.sender == owner should be a modifier since it is repeated many times in the code

• productCounter only increasing will affect getProducts(), might reach out of gas error

• bondCounter only increasing will affect getBonds(), might reach out of gas error

• payout may truncate to zero with extremely small tokenAmount

Other Contracts

Centralization risks

• The owner controls most of the flow of the protocol.

• Owner can set all addresses, have capabilities like changeOwner and changeManager

• Owner also controls the amount of supply for a given LP bond, and controls the tokenomicsParameters

• Owner cannot withdraw funds from the user, which is a good thing, but since the owner controls the protocol, the protocol still have a high centralization risk

• Centralization Risk - High

Architecture Review

Design Patterns and Best Practices:

There is good usage of established design patterns, like pausing mechanism and reentrancy guard. There is also consistent checks for zero values and overflows. This shows that the protocol is well versed in security.

One good example is the usage of allowance check in Treasury.sol. Even though the allowance check is not needed because transferFrom inherently check for allowance, the function still checks because Uniswap allowance implementation does not revert with the accurate message. This shows that the protocol understands how external functionality works well.

Code Readability and Maintainability:

Functions are pretty difficult to understand, but the NATSPECs does help in facilitating understanding. Code can sometimes be repetitive, such as the repeated checking of owner. Some overflow checks are also redundant because of 0.8.0 where overflows will automatically be reverted.

Error Handling and Input Validation:

Events and Error messages are easy to understand. Error messages are also written neatly at the top of the contract, with sufficient NATSPEC explaining the reasoning for the error.

Interoperability and Standards Compliance:

Good knowledge of the ERC20 standard when creating the OLAS tokens. This is seen from protocol checking for success value after transferFrom, and creating a soft max cap with 2% inflation rate for their token.

Testing and Documentation:

Extensive tests (unit, integration tests) done, reaching an overall coverage of almost 100%. Documentation (whitepaper, github) is plentiful and includes quality reasoning at every juncture of the protocol. One improvement could be having more diagrams and a summarized version of how the protocol works from the top down, with a end-to-end scenario of how a user can interact with the protocol (what function should the user call first, what can a user accomplish, why is the user incentivized to use the protocol)

Upgradeability:

Most of the protocol is not intended to be upgradeable, but contracts can be redeployed and rerouted easily through the changeManagers and changeRegistries function.

Dependency Management:

Protocol relies on external libraries like OpenZeppelin, and also uses code from other contracts (veCRV, FXPortal, Arbitrary Message Bridge). If those code are not well secured, it will affect the protocol. Protocol should keep an eye on vulnerabilities that affects those external integrations, and make changes where necessary.

Overall, great architecture from the protocol, slight changes would be to the written code itself (using modifiers for repeated code, checking zero values, checking overflows etc) and more explicit documentation

Comments and Context

• Due to time constraints, the audit only focused on the Bonding and Voting Escrow portion of the protocol. The contracts being focused on are OLAS.sol, veOLAS.sol, wveOLAS.sol, GovernorOLAS.sol, Depository.sol, Dispenser.sol, DonatorBlacklist.sol, GenericBondCalculator.sol, Tokenomics.sol, TokenomicsConstants.sol, TokenomicsProxy.sol, Treasury.sol. For the rest of the contracts, I have looked only looked briefly just to contextualize the whole protocol.

Time spent:

025 hours