Reach out for an audit or to learn more about Macro
or Message on Telegram

Tales of Elleria A-1

Security Audit

June 19, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Tales of Elleria's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from June 5, 2023 to June 6, 2023.

The purpose of this audit is to review the source code of certain Tales of Elleria Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 1 - - 1
Medium 3 - - 3
Low 7 - - 7
Code Quality 5 - - 5
Gas Optimization 2 - - 2

Tales of Elleria was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository:

Contract SHA256
contracts/ElleriaElmBridge.sol

33ed5c11c27259aedb983c6068e561822f3686bc9091c872a799e074700e5b90

contracts/ElleriumLPStakingPool.sol

22de94f8ddcae94c8aadcb2544d317405e9edf069b248f8ccee6ea3f38026f52

contracts/VerifySignature.sol

a443de278f432f423622a6c51b0c55a8b085335870d87f550be3e0fbc9ddb533

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

Hero owner may reset Hero properties

Topic
Spec
Status
Addressed
Impact
High
Likelihood
High

In the EllerianHeroUpgradeable contract, SynchronizeHero() function may be invoked multiple times with the same signature to reset Hero related attributes to previously valid but currently obsolete data.

VerifySignature contract documentation indicates that its verification functions, such as bigVerify() on which SynchronizeHero() relies, do not have replay attack protection and that any logic reliant must also have logic guarding against replay attacks.

However, SynchronizeHero() does not follow this guideline and therefore is vulnerable to replay attack.

Remediations to Consider:

  • Implement replay attack protection similar to how it is implemented in ElleriaElmBridge:RetrieveElleriumFromGame() using a nonce as part of the signed message.
Response by Tales of Elleria

This will be deprecated, and the source of truth is now our backend servers instead to support custom in-game functionality.

The backend servers will be integrated with Chainlink’s VRF to proof state changes to ensure reliability and integrity.

M-1

Incorrect output of checkTotalRewards() function

Topic
Spec
Status
Impact
Medium
Likelihood
Medium

In the ElleriumLPStakingPool contract, rewardRate value is denominated in Gwei. In addition, setRewardRate(), the function for updating rewardRate expects input denominated in Gwei _emissionRateInGWEI. Also, in getReward() function totalRewards[msg.sender] is multiplied by 1e9 to get the actual reward amount in Wei.

function getReward() public nonReentrant updateReward(msg.sender) {
    uint256 reward = totalRewards[msg.sender] * 1e9;
    if (reward > 0) {
        totalRewards[msg.sender] = 0;

        rewardsToken.mint(msg.sender, reward);
        emit ClaimedReward(msg.sender, reward);
    }
}

However, the implementation of the checkTotalRewards() function does not have corresponding multiplication by the 1e9 factor, even though the natspec comment indicates that the return value is denominated in Wei.

/// @notice Exposes sender's claimable rewardToken balance.
/// @return Sender's Claimable rewardToken balance in WEI.
function checkTotalRewards() external view returns (uint256) {
    return totalRewards[msg.sender];
}

Remediations to Consider:

  • Update checkTotalRewards() so it returns the totalRewards amount for a particular user denominated in Wei instead of Gwei, or
  • Change rewardRate to be expressed in Wei instead of Gwei. Correspondingly, update setRewardRate() and getReward() function implementations to work with Wei denomination to prevent accidental issuance of enormous rewards which could happen at the moment if setRewardRate() is called with an emission rate incorrectly provided in Wei.
M-2

Contract dependencies are not immutable

Topic
Trust model
Status
Impact
Medium
Likelihood
Medium

In the ElleriumLPStakingPool and ElleriaElmBridge contracts, setAddresses() and SetReferences() functions can be called only by the contract owner. These two functions are responsible for initializing and updating important external contract dependencies, which include staking token, rewards token, staking fee, and signature verification contract.

If these contract dependencies are not properly set or they get accidentally misconfigured, overall system operation will be affected. In addition, the two previously mentioned contracts cannot be properly used after deployment without a call to these functions.

To increase user trust in the overall system, it is important to provide means for users to verify important state changes and to react in case they disagree with the same. Also, it is best practice to minimize important system configuration updates, such as updates to external dependencies, unless they are necessary.

Remediations to consider:

  • Set contract dependencies in the contract constructor during contract deploy, and
  • In case contract dependencies updates are required, add a timelock mechanism with appropriate delay when contract dependency updates are performed in an existing system.
Response by Tales of Elleria

Contracts within scope have been modified to only set dependencies in the constructor. Subsequent contracts that are single-utility will also be switched to follow the same pattern.

M-3

Contract owner has excessive privileges for managing token assets

Topic
Trust model
Status
Impact
Medium
Likelihood
Medium

In the ElleriumLPStakingPool and ElleriaElmBridge contracts, the withdrawToken() function, which is callable only by the owner, enables the caller to withdraw all ERC20 assets from the contract.

function withdrawToken(address _tokenContract, uint256 _amount) external onlyOwner {
  IERC20 tokenContract = IERC20(_tokenContract);
  tokenContract.transfer(msg.sender, _amount);
}

While this functionality may serve a purpose in an emergency situation, its presence in its current form may affect users’ trust in the overall system.

Remediations to Consider:

  • Consider removing withdrawToken() from the ElleriaElmBridge since ElleriaElmBridge does not store ELM assets but burns them during BridgeElleriumIntoGame() operation.
  • Consider replacing withdrawToken() in ElleriumLPStakingPool with functionality for individual users to perform an emergency withdrawal in case an emergency state is enabled.
Response by Tales of Elleria

Resolutiom: withdrawToken() removed from both contracts.

The contract ElleriaElmBridge does not manage ERC20 tokens, hence there was no risk of funds within the contract being mismanaged.

ElleriumLPStakingPool, the intent was to withdraw stuck tokens, though makes sense it can cause trust issues.

The "emergency state" is essentially RewardRate set to 0 and feesTaken set to false.

L-1

Unsafe usage of ERC20 transfer functions

Topic
Coding standards
Status
Impact
Low
Likelihood
Low

In the following instances, return values of transfer are ignored:

  • ElleriumLPStakingPool:stake()
  • ElleriumLPStakingPool:unstake()
  • ElleriumLPStakingPool:withdrawToken()
  • ElleriaElmBridge:withdrawToken()
  • ElleriaElmBridge:BridgeElleriumIntoGame()

For handling non-standard ERC20 token implementations which do not revert, but return false on transfer failure, consider using OZ’s SafeERC20 variants of token handling functions safeTransfer() / safeTransferFrom().

Remediations to Consider:

  • Instead of IERC20’s transfer() and transferFrom(), consider using OZ’s SafeERC20 variants safeTransfer() / safeTransferFrom().
Response by Tales of Elleria

Additional updates in commit 50bdf09d1779fd76caeaaaf6d7dbd4fce5320f9b.

L-2

Incorrect EIP-712 implementation

Topic
Coding standards
Status
Impact
Low
Likelihood
Low

The VerifySignature contract implements signature verification based on the EIP-712 standard.

However, the current implementation does not include DOMAIN_SEPARATOR as part of the signature. As a result, signatures generated on one chain may be reused on other chains.

Remediations to Consider:

  • Implement the EIP-712 standard with corresponding DOMAIN_SEPARATOR verification functionality.
L-3

Missing argument in ERC20Withdraw event

Topic
Events
Status
Impact
Low
Likelihood
Low

In the ElleriaElmBridge when RetrieveElleriumFromGame() is executed corresponding ERC20Withdraw event is emitted. This event contains info about the caller, ELM token contract, the amount withdrawn, and the current number of withdrawals.

However, the ERC20Withdraw event does not contain _txnCount information, which is practically the id of the withdrawal transaction. As a result, it is harder to determine which previously signed transactions by the internal server have already been processed on-chain.

Remediations to consider:

  • Add _txnCount information to the ERC20Withdraw event.
Response by Tales of Elleria

Additional updates in commit 50bdf09d1779fd76caeaaaf6d7dbd4fce5320f9b.

L-4

Signature replay using hash collision

Topic
Signature
Status
Addressed
Impact
Low
Likelihood
Low

In the EllerianHeroUpgradeable contract, SynchronizeHero() function implementation relies on a fixed set of elements (11) from provided _data array argument. This _data argument is also forwarded to VerifySignature:bigVerify() function which iterates through _data elements and calculates message hash for signature verification.

However, since the _data array can be of a length that is greater than the necessary set of elements, the malicious caller of SynchronizeHero() can add extra elements to the _data array to generate a hash collision.

Remediations to consider:

  • Add guard condition to prevent SynchronizeHero() calls with _data array length greater than required.
Response by Tales of Elleria

Function to be deprecated.

L-5

Missing events for important state updates

Topic
Events
Status
Impact
Low
Likelihood
Low

In the ElleriumLPStakingPool and ElleriaElmBridge contracts, several important state-changing functions miss corresponding event emissions. These are:

  • ElleriaElmBridge:SetReferences()
  • ElleriaElmBridge:withdrawToken()
  • ElleriumLPStakingPool:setAddresses()
  • ElleriumLPStakingPool:setRewardRate()
  • ElleriumLPStakingPool:withdrawToken()

However, without appropriate events, it is difficult for off-chain tools to track and monitor on-chain system behavior.

Remediations to consider:

  • To facilitate off-chain tracking and monitoring, introduce appropriate events for important system state changes.
Response by Tales of Elleria

Changed variables for the dependencies to be public.

Contract dependencies set only within the constructor.

L-6

Non-standard ERC20 burn implementation

Topic
Coding standards
Status
Impact
Low
Likelihood
Low

In the ElleriumTokenERC20v2 contract, the internal _transfer() function is meant to handle the transfer to 0 address as a burn operation. When recipient is address(0), totalSupply is reduced by the provided amount of token.

// Demint for burn transactions.
if (recipient == address(0)) {
    _totalSupply = _totalSupply - amount;    
}

_balances[actualRecipient] = _balances[actualRecipient] + amount;

However, what differs from expected behavior is that _balance of address(0) will still get increased by the provided amount. This not be the case since it invalidates the invariant that the sum of individual account balances should be equal to the totalSupply.

Remediations to Consider:

  • Update implementation so that the balance update above is placed in the else block.
L-7

Incorrect Transfer event data

Topic
Coding standards
Status
Impact
Low
Likelihood
Low

In the ElleriumTokenERC20v2 contract, the internal _transfer() function performs token transfer with additional logic to prevent transfers before the system is completely configured. One aspect of this mechanism is that those who attempt transfers beforehand will get blacklisted. A consequence of getting blacklisted is that transfers from the blacklisted address will be rerouted to the owner-controlled address.

When a transfer from the blacklisted address is processed, corresponding balances are updated. However, Transfer event emission references the initial recipient instead of actualRecipient which in this edge case is incorrect.

...
_balances[actualRecipient] = _balances[actualRecipient] + amount;
emit Transfer(sender, recipient, amount);

Remediations to Consider:

  • Update Transfer event emission to reference actualRecipient instead of the recipient.
G-1

Avoid taxFee transfer calls when there is no taxFee

Topic
Best practices
Status
Gas Savings
Medium

In the ElleriumLPStakingPool:unstake() implementation, the taxFee transfer call always happens.

stakingToken.transfer(stakingLPFeesAddress, taxFee);

However, this call is unnecessary in the following situations:

  • applyFees is false
  • taxFee is 0
  • stakingLPFeesAddress is address(0)
G-2

Prefer += instead of + since it costs less gas

Topic
Best practices
Status
Gas Savings
Low

When updating a value in an array with arithmetic, using array[index] += amount is cheaper than array[index] = array[index] + amount. This is because you avoid an additional mload when the array is stored in memory and a sload when the array is stored in storage.

This can be applied for any arithmetic operation including +=, -=,/=,*=,^=,&=, %=, <<=,>>=, and >>>=. This optimization can be particularly significant if the pattern occurs during a loop.

Saves 28 gas for a storage array, 38 for a memory array

Instances (4):

File: ETH_ElleriumTokenERC20.sol

229:         _balances[actualRecipient] = _balances[actualRecipient] + amount;

242:         _balances[account] = _balances[account] + amount;
File: ElleriumLPStakingPool.sol

129:         balances[msg.sender] = balances[msg.sender] + _amount;

144:         balances[msg.sender] = balances[msg.sender] - _amount;
Q-1

Use Solidity’s custom errors

Topic
Errors
Status
Quality Impact
Low

Consider using Solidity’s 0.8.4 feature - custom errors to reduce contract size, increase code readability, and have more gas-efficient operations.

Response by Tales of Elleria

Implemented for contracts under scope.

Q-2

require() / revert() statements should have descriptive reason strings

Topic
Best practices
Status
Quality Impact
Low

Instances (3):

File: ETH_ElleriumTokenERC20.sol

61:         require (msg.sender == owner() || _approvedAddresses[msg.sender]);
File: ElleriumLPStakingPool.sol

122:         require(_amount > 9999);

140:         require(_amount <= balances[msg.sender]);
Q-3

Consider following standard function naming conventions

Topic
Best practices
Status
Quality Impact
Low

In some contracts, function names are capitalized, while in others, they follow the standard camelCase naming convention. Consider using the camelCase convention consistently throughout the whole codebase.

Response by Tales of Elleria

Implemented for contracts under scope, to maintain the standard across all other contracts as well.

Q-4

Remove redundant code for calculating and applying staking taxFee

Topic
Best practices
Status
Quality Impact
Low

In the ElleriumLPStakingPool same logic for calculating staking taxFee is implemented in two different ways in the following two functions:

  • getWithdrawalFees()
  • unstake()

Consider reusing getWithdrawalFees() in unstake() instead of reimplementing it.

Q-5

Consider improving the Natspec documentation

Topic
Documentation
Status
Quality Impact
Low
  • Add natspec comments for contract variables
  • Add natspec for missing function arguments and return variables
  • Add natspec comments for event declarations and their arguments
  • (If custom errors are implemented) Add natspec comments for custom errors
Response by Tales of Elleria

Changed for contracts under scope.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Tales of Elleria team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.