Savea A-1

In the Management contract, the sellTokensForCurrency() allows redemption of Savea tokens up to the Settings.REDEEM_PERCENTAGE per month. See isReedeemable() , _checkandUpdateRedemption() and checkRedemptionAllowance(). When redemption is enabled, and the user tries to redeem for the first time within a particular month, the redemption allowance for that period is calculated and recorded.

// checkRedemptionAllowance()
if (redemption.amountRedeemed == 0) {
    // wallet didn't reedem this month so they can request the full redemption amount
    return ((token.balanceOf(msg.sender) *
        (settings[Settings.REDEEM_PERCENTAGE])) / 10 ** 20); // 20 is required to get to the correct decimals
}

In the _checkAndUpdateRedemption(), this allowance is decremented for the redeemed amount while the redeemed amount is tracked separately. On subsequent redemption attempts during the same month, the user’s amountAllowed is checked, and if it is greater or equal to the requested amount, the request is processed; otherwise, it is denied.

However, the current implementation approach's issue implicitly assumes that users cannot transfer their tokens and that their token balance is fixed.

Malicious users may redeem up to REDEEM_PERCENTAGE of tokens from one address, transfer the remaining balance to a different address, and repeat the process in the loop until the whole balance is redeemed. An alternative approach for them is to move the aggregated balance iteratively to multiple addresses and perform redemption with one wei to obtain max REDEEM_PERCENTAGE at each address. Afterward, they can split their token balance across those addresses and redeem it in one step.

Remediations to Consider

• Limit token transfers for a particular token holder and the current period if the redemption request has already been processed.

In the Management contract, chargeFeesAllBatch() efficiently enables iterative processing and fee collection from all token holders. Related chargeFeesAll() function performs the identical operation. However, it is unsuitable when there are many token holders; therefore, chargeFeesAllBatch() is expected to be used more commonly.

However, in the case of chargeFeesAllBatch(), users may exploit the fact that fee collection is performed in multiple transactions across different groups of token holders. Users may observe that the owner has a pending transaction chargeFeesAllBatch(), and they may front-run it with a transfer of their balance to the account address under their control but not specified as part of the batch to be processed. The token balance fee will not be applied as their initial address will now have 0 balance. Immediately after chargeFeesAllBatch() is complete, they transfer their tokens back to the initial address, which usually would not be processed again during the same time period.

This issue impacts the core invariant of charging fees on all token balances not stored with the Management contract.

Remediations to consider

• Redesign the fee collection process. For example, in one approach, the system could mint a token equal to the fee percentage of all tokens in circulation and assign this newly minted asset balance to the Management contract.
• An alternative approach would be to use a rebase token system similar to stETH, where the token tracks the individual tokenholder's share. Then, based on changes in total supply (due to fee collection) meant for token holders, individual holder balance changes.

In the Management contract, _chargeFee() internal function is responsible for processing and recording fee collection for token holders. Implementation check if time delta between current block.timestamp and previous fee collection timestamp is greater than the defined feeStructure.period for fee collection.

/**
* @notice Internal functon to charge a fee
@param wallet Address of the wallet to charge
**/
function _chargeFee(address wallet) internal {
    // this contract will hold the tokens but we don't want to charge fees on them
    if (wallet != address(this)) {
        // check the last payment date to see if the fees are due
        if (block.timestamp - payments[wallet] > feeStructure.period) {

            uint256 feeAmount = ((token.balanceOf(wallet) / 100) *
                feeStructure.fee) / 10 ** token.decimals();

            if (feeAmount > feeStructure.minimumTokens) {
                // charge the wallet
                token.update(wallet, address(this), feeAmount);

                // update the last payment date
                payments[wallet] = block.timestamp;

                // emit event
                emit FeeCharged(wallet, feeAmount);
            }
        }
    }
}

However, when the holder is processed for the first time, their payments[wallet] record will be 0 as it is not initialized. As a result, they would be eligible for fee collection since the time delta in this scenario would certainly be greater than the value of feeStructure.period. Consequently, when transferred between multiple token holders, the same token balance would be accounted for fee collection, resulting in excess fee charges per period.

Remediations to Consider

• Redesign fee collection system as described in [H-2].

In the Management contract, buyTokensWithCurrency() and sellTokensForCurrency() functions enable users to buy and sell Savea tokens in exchange for listing currency. Both of these functions rely on a fixed exchange rate defined by Settings.EXCHANGE_RATE_BUY and Settings.EXCHANGE_RATE_SELL to perform conversion. The contract owner controls these two settings and updates when necessary.

However, when the exchange rate deviates from the configured value, external actors may exploit arbitrage opportunities and buy or sell tokens at favorable value. This opportunity exists even if the exchange rate value is updated frequently since malicious actors may front-run price update transactions.

Remediations to Consider

• Update implementation to rely on external oracles provided by 3rd parties (e.g. zk oracles, chainlink, etc.)

In the Management contract, the buyTokensWithCurrency() function enables users to buy Savea tokens. This function contains several input validations, including a check, which validates if the contract’s token balance is enough to service the pending buy request.

// calculate the token amount into the correct token decimals
uint256 tokenAmount = (currencyAmount * 10 ** token.decimals()) /
    settings[Settings.EXCHANGE_RATE_BUY];

// check if the contract has enough tokens
require(
    token.balanceOf(address(this)) >= currencyAmount,
    Errors.MANAGEMENT_AVAILABLE_TOKENS
);

However, the check implementation references the wrong variable, currencyAmount, instead of tokenAmount. If the exchange rate is such that one token is worth much more than currency, it may prevent processing purchase requests even if the contract has enough token balance.

Remediations to Consider

• Update check implementation

// check if the contract has enough tokens
require(
    token.balanceOf(address(this)) >= tokenAmount,
    Errors.MANAGEMENT_AVAILABLE_TOKENS
);

In the Management contract, checkRedemptionAllowance() determines the amount of tokens a specific wallet can sell within a particular redemption period.

/**
 * @notice Check the redemption allowance for the wallet
 * @param wallet Wallet address to check
 */
function checkRedemptionAllowance(
    address wallet
) public view returns (uint256 amountAllowed) {
    // get redemption for address in the current month
    DataTypes.Redemption storage redemption = redemptions[wallet][
        settings[Settings.CURRENT_MONTH]
    ];

    if (redemption.amountRedeemed == 0) {
        // wallet didn't reedem this month so they can request the full redemption amount
        return ((token.balanceOf(msg.sender) *
            (settings[Settings.REDEEM_PERCENTAGE])) / 10 ** 20); // 20 is required to get to the correct decimals
    } else {
        // wallet already reedemed this month so they can request up to the amount allowed already set
        return (redemption.amountAllowed);
    }
}

However, while the function accepts the wallet as a parameter, it performs calculations based on the token balance of msg.sender instead of the wallet address. As a result, the returned value for the provided wallet will differ from the correct one if the wallet and msg.sender are unequal.

Remediations to Consider

• Update implementation to reference to token.balanceOf(wallet) instead of token.balanceOf(msg.sender), or
• Remove wallet parameter and checkRedemptionAllowance() only for msg.sender

In the Management contract, the owner may update important system configurations using the updateFeeStructure() function. When the fee is set, it affects which percentage of assets is charged per period from each token holder balance.

However, if this fee is by accident or due to a compromise set to an unexpectedly high value (e.g., 100%), it may result in a loss of assets for the end users.

Remediations to Consider

• Add limits (min, max) for the most important system configuration variables to indicate expected system conditions and reduce risks for managed assets.

In the Management contract, the withdrawNFTOnly() and withdrawAndBurnNFT() functions enable users to withdraw their NFT token. As part of the implementation, NFTWithdraw() event is emitted.

/** Emitted when nfts are withdrawn */
event NFTWithdraw(address to, address token, uint256 id);

/* Event emission in withdrawNFTOnly() and withdrawAndBurnNFT() */
emit NFTWithdraw(to, address(token), tokenId);

However, both functions do not use a token (the address of the Savea token) but a nftContract associated with a specific token. As a result, second emitted parameter is incorrect and misleading for off-chain tracking and monitoring tools relying on event data.

Remediations to Consider

• Update the implementation of withdrawNFTOnly() and withdrawAndBurnNFT() to emit proper event data and event definition to more clearly indicate what is being emitted.

/** Emitted when nfts are withdrawn */
event NFTWithdraw(
    address indexed to,
    address indexed nftContract,
    uint256 indexed tokenId
);

/* Event emission in withdrawNFTOnly() and withdrawAndBurnNFT() */
emit NFTWithdraw(to, nftContract, tokenId);

The following public view functions are access controlled in SaveaToken.sol

• getTokenHolderSize()
• getTokenHolderAtIndex()
• getTokenHolderState()

Consider removing the onlyRole(MANAGEMENT_ROLE) modifier since it is unnecessary.

In the Management contract, the following code is present but unused:

• commented out code
  • checkBuyMinimum
  • checkBuyMaximum
• unused code
  • validateAddress() modifer
  • checkAvailableFunds() modifier
  • Errors.MANAGEMENT_FEES_INVALID_ADDRESS

Consider removing this code unless it is needed.

The Management contract, which relies heavily on a centralized owner, should support a safe owner transition to a more appropriate privileged operation handling account, such as one using a multi-sig. Ownable2Step can enable safer owner transition when it is necessary.

The Management contract, which inherits from OpenZeppelin’s Ownable contract, has renounceOwnership() functionality. When this function is called, it permanently removes ownership of the contract.

Since Savea’s Management contract is highly centralized and cannot operate without an owner, consider disabling the renounceOwnership() functionality by overriding it and reverting whenever it is called.

In the Management contract, the buyTokensWithCurrency() emits TokensPurchased event:

emit TokensPurchased(msg.sender, currencyAmount, tokenAmount);

When TokensPurchased event is defined as following with parameter names which are misleading or inadequately named.

event TokensPurchased(
    address indexed wallet,
    uint256 price,
    uint256 tokens
);

For comparison, TokenSold event has more appropriate parameter naming.

event TokensSold(
    address indexed seller,
    uint256 tokenAmount,
    uint256 currencyAmount
);

Consider updating TokensPurchased event emission.

In the Management contract, for general interaction with token and currency addresses, consider using SafeERC20 to help deal with various ERC20 implementations unless they are under system control.

In the Management contract, different numerical values are used as literals in several function implementations. While there is no currently negative impact on the system operation, consider replacing these literal values with well-defined constants. In the constructor, set immutable variable TOKEN_PRECISION.

• checkRedemptionAllowance() - replace 1020 with TOKEN_PRECISION * PERCENTAGE_PRECISION (1018 * 100)

  return ((token.balanceOf(msg.sender) *
     (settings[Settings.REDEEM_PERCENTAGE])) / 10 ** 20); 
  

• buyTokensWithCurrency() - replace 10 ** token.decimals() with TOKEN_PRECISION

  uint256 tokenAmount = (currencyAmount * 10 ** token.decimals()) /
      settings[Settings.EXCHANGE_RATE_BUY];
  

• sellTokensForCurrency() - replace 10 ** token.decimals() with TOKEN_PRECISION

  uint256 currencyAmount = (tokenAmount *
      settings[Settings.EXCHANGE_RATE_SELL]) / 10 ** token.decimals();
  

In the Management contract, fee collection is performed using chargeFeesAllBatch() and chargeFeesAll(), triggered by the owner for each period. The cost of operating, charging, and collecting fees is manageable for a few token holders.

However, the current implementation would not be scalable for any larger number of token holders, especially in the scenario of high gas fees on the Ethereum mainnet, where the project is currently deployed.

Consider redesigning the fee collection system according to the design suggested in [H-2], as in that case, the complexity of operation would be O(1) instead of O(n).

The contracts in scope, SaveaToken.sol and Management.sol, feature extensive functionality under the system owner's direct control. In case of owner account compromise or incidental and/or malicious owner actions, user assets could be affected.

Consider implementing various mechanisms for a more decentralized and less trust-demanding system:

1. use a multi-sig account for an owner account instead of EOA, and enhance with governance features if possible
2. consider introducing a timelock mechanism for important system settings updates, so that end users may have the opportunity to react to undesirable system changes
3. introduce features that allow the system to be more decentralized, such as oracle for tracking exchange rate.
4. remove or set limits on owner operations and handling of user assets such as transfer of tokens, transfer of currency assets, and transfer of NFT tokens.

In the Management contract, the Settings.CURRENT_MONTH parameter is used to access various configuration parameters for each month. If CURRENT_MONTH always increases and never loops, there is no danger that some previously used and updated config variables will be reused in the new period. However, if that is not the case, it may negatively affect system behavior.

Consider treating CURRENT_MONTH as a monotonically increasing value and do not reset it.