EVM Smart Contracts - Secta Finance

The usages of the permit ERC20 extension in the code can be frontrun by any party that witnesses transactions prior to block inclusion. This is a griefing vector that will cause user transactions to fail if the associated permit has already been consumed. See this article for more details. While the risk of this seems quite low (very little incentive to perform the attack, and if the centralized sequencer is trustworthy, it will not grief users or reveal their transaction inappropriately), it could cause a bad experience for users.

Consider implementing an allowance check as a fallback if permit validation fails, so that transactions for which the permit was already consumed do no revert.

Remediation Plan

RISK ACCEPTED: The Secta Finance team accepted this risk of this finding.

Multiple functions across the protocol codebase make use of the Ethereum-specfic ecrecover function directly to verify the given signatures. However, using ecrecover directly allows malleable (non-unique) signatures and thus is susceptible to replay attacks. Although replay attacks are not possible as the contracts correctly use different mitigations (domain separator, nonces...), ensuring the signatures are not malleable is considered a best practice (and so is checking that the_signeris different than the address(0), where address(0) means an invalid signature).

It is recommended to use the recover function from OpenZeppelin's ECDSA library for signature verification.

Remediation Plan

RISK ACCEPTED: The Secta Finance team accepted this risk of this finding.

Multiple contracts are using the Initializable module from OpenZeppelin. In order to prevent leaving the contracts uninitialized OpenZeppelin's documentation recommends adding the _disableInitializers function in the constructor to automatically lock the contracts when they are deployed:

Consider calling the _disableInitializers function in all the contracts' constructor.

Remediation Plan

RISK ACCEPTED: The Secta Finance team accepted this risk of this finding.

Contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Consider locking the pragma version in the smart contracts. It is not recommended to use a floating pragma in production.

For example: pragma solidity 0.6.12;

Remediation Plan

ACKNOWLEDGED: The Secta Finance team acknowledged this finding.

If a loop is iterated many times, the amount of gas required to execute the function increases significantly. In Solidity, excessive looping can cause a function to use more than the maximum allowed gas, which causes the function to fail.

To reduce gas consumption, it is recommended to find ways to optimize the loop or potentially break the loop into smaller batches. The following pattern can also be used:

uint256 cachedLen = array.length;
for(uint i; i < cachedLen;){
  unchecked {
    ++i;
  }
}

Remediation Plan

ACKNOWLEDGED: The Secta Finance team acknowledged this finding.

The current ownership transfer process for all the contracts inheriting from Ownable or OwnableUpgradeable involves the current owner calling the transferOwnership() function:

function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    _setOwner(newOwner);
}

If the nominated EOA account is not a valid account, it is entirely possible that the owner may accidentally transfer ownership to an uncontrolled account, losing the access to all functions with the onlyOwner modifier.

It is recommended to implement a two-step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account. This can be easily achieved by using OpenZeppelin's Ownable2Step contract instead of Ownable:

Remediation Plan

ACKNOWLEDGED: The Secta Finance team acknowledged this finding.

In the IFOV2 contract some variables are reinitialized to 0:

As userRefundingAmount and taxAmountare uint256, they are already initialized to 0. userRefundingAmount = 0 reassigns the 0 to userRefundingAmount which wastes gas.

It is recommended to not reinitialize the suggested local variables in order to reduce gas costs.

Remediation Plan

ACKNOWLEDGED: The Secta Finance team acknowledged this finding.