Spirals Audit Report

1. Architecture Consultancy Service
   • Protocol Security & Design Discussion Meeting
     • As a part of the audit service, the Verilog Solutions team worked closely with the Spirals development team to discuss potential vulnerability and smart contract development best practices in a timely fashion. Verilog Solutions team is very appreciative of establishing an efficient and effective communication channel with the Spirals team, as new findings were exchanged promptly and fixes were deployed quickly, during the preliminary report stage.

2. Smart Contract Auditing Service
   • The Verilog Solutions team analyzed the entire project using a detailed-oriented approach to capture the fundamental logic and suggested improvements to the existing code. Details can be found under Findings & Improvement Suggestions.

• Code Assessment
  • We evaluate the overall quality of the code and comments as well as the architecture of the repository.
  • We help the project dev team improve the overall quality of the repository by providing suggestions on refactorization to follow the best practice of Web3 software engineering.

• Code Logic Analysis
  • We dive into the data structures and algorithms in the repository and provide suggestions to improve the data structures and algorithms for the lower time and space complexities.
  • We analyze the hierarchy among multiple modules and the relations among the source code files in the repository and provide suggestions to improve the code architecture with better readability, reusability, and extensibility.

• Access Control Analysis
  • We perform a comprehensive assessment of the special roles of the project, including their authorities and privileges.
  • We provide suggestions regarding the best practice of privilege role management according to the standard operating procedures (SOP).

• Off-Chain Components Analysis
  • We analyze the off-chain modules that are interacting with the on-chain functionalities and provide suggestions according to the SOP.
  • We conduct a comprehensive investigation for potential risks and hacks that may happen on the off-chain components and provide suggestions for patches.

1. activeVotes ≥ value

   Severity	Medium
   File	src/staking/SpiralsStakingV2.sol#L93;
   Commit	dc3d7e3;
   Status	Resolved in commit ee1c18c;

   • Description

     The only requirements are activeVotes + pendingVotes >= _value, so there are following combinations:

     activeVotes	pendingVotes
     ≥ _value	≥ _value
     ≥ _value	≤ _value
     ≤ _value	≥ _value
     ≤ _value	≤ _value

     In the if-else condition, when activeVotes ≤ _value, pendingVotes can be bigger or smaller or equal to _value.

     if (activeVotes >= _value) {
           revokeActive(_value);
       } else {
           revokePending(_value);
       }
       
       unlock(_value);

     So in the else condition, the revoke pending will fail when pendingVotes < value.

   • Exploit Scenario

     1. Alice has less pendingVotes and activeVotes than the value that she wants to unstake, but the condition activeVotes + pendingVotes >= _value is still fulfilled.

     2. Alice calls the unstake() function since the property of the sum is fulfilled.

     3. The revokePending() function will fail.
   • Recommendations

     Add check to guarantee that pendingVotes >= value when activeVotes ≤ _value, i.e., contemplate this case in the else condition of the if statement.

   • Results

     Resolved in commit ee1c18c;

2. Yield cannot be locked in the SpiralsCUSDVaultMigration contract

   Severity	Medium
   File	src/staking/migration/SpiralsCUSDVaultMigration.sol;
   Commit	d7debb4;
   Status	Resolved in commit 6dcce64;

   • Description

     The yields cannot be withdrawn from the current CUSD vault and the migration only migrates the principals for users to the new impact vault. A function should be added to the contract to withdraw the yields from the current contract.

     The SpiralsCUSDVaultMigration.sol contract is a vault that supports yield-bearing tokens like mcUSD. User can only withdraw their principals back because of the fixed 1:1 asset and staked asset ratio. Their balance of vault token (SpiralsCUSDVaultMigration.balanceOf()) will always be the principal amount.

     When doing the migration for the CUSD vault, the SpiralsCUSDVaultMigration.transferAssetsToImpactVaultForAddress() will withdraw CUSD from the Moola lending pool and deposit the CUSD into the new impact vault for users. However, only principals are deposited into the new impact vault for users because the migration amount for the user is read from the function SpiralsCUSDVaultMigration.balanceOf() directly. So after migration, the yields will still stay in the current CUSD vault and no one can touch it. A function should be added to deal with those yields left in the contract after migration.

   • Exploit Scenario

     1. Owner executes migration.

     2. All users principles are withdrawn and deposited for them to the new impact vault.

     3. There rest of the staked asset in vault (yield) will still be locked in the vault.
   • Recommendations

     Calculate the respective yield for each of the users that have staked and send that yield to the ImpactVault contract.

   • Results

     Resolved in commit 6dcce64.

     Now the yield can be manual passed into the migration functions in both the SpiralsCUSDVaultMigration.sol and SpiralsStakingMigration.sol contracts to transfer it to the ImpactVault. As it is not easy to track the precise amount of yields each user earned, Spirals team decided to do an approximate calculation. The generated yields will be assigned to users proportionally to their share.

1. Unused events

   Severity	Low
   File	src/vaults/ImpactVaultManager.sol#L33, L34, L35; src/vaults/SpiralsCUSDImpactVault.sol#L24;
   Commit	dc3d7e3;
   Status	Resolved in commit ee1c18c;

   • Description

     Those specified events are defined but not used.

   • Exploit Scenario

     N/A.

   • Recommendations

     Use events when applicable. Operations that transfer value or perform critical operations should trigger events so that users and off-chain monitoring tools can account for important state changes.

   • Results

     Resolved in commit ee1c18c;

2. Incorrect event emission for critical operations

   Severity	Low
   File	src/vaults/ImpactVault.sol#L172;
   Commit	dc3d7e3;
   Status	Resolved in commit 90291c1;

   • Description

     Function withdrawYieldAsset() emits the same event as the withdraw() function. However, they perform different operations, namely withdraw() transfers the underlying staked asset to the receiver, and withdrawYieldAsset() transfers the yield asset. The WithdrawYield event is defined and should be used in the line specified.

   • Exploit Scenario

     N/A.

   • Recommendations

     Emit WithdrawYield event instead of Withdraw when a user withdraws yield.

   • Results

     Resolved in commit 90291c1.

     Event and function were renamed for more clarity.

1. Floating solidity pragma version

   Severity	Informational
   File	Global;
   Commit	dc3d7e3;
   Status	Resolved in commit 90291c1;

   • Description

     Current smart contracts use ^0.8.10. And compilers within versions ≥ 0.8.10 and <0.9.0 can be used to compile those contracts. Therefore, the contract may be deployed with a newer or latest compiler version which generally has higher risks of undiscovered bugs.

     It is a good practice to fix the solidity pragma version if the contract is not designed as a package or library that will be used by other projects or developers.

   • Exploit Scenario

     N/A.

   • Recommendations

     Use the fixed solidity pragma version.

   • Results

     Resolved in commit 90291c1.

     The solidity pragma version is now fixed to 0.8.11.

2. Unused Custom error

   Severity	Informational
   File	src/vaults/ImpactVault.sol#L35;
   Commit	dc3d7e3;
   Status	Resolved in commit 90291c1;

   • Description

     Custom error in line specified is defined but not used.

     error WithdrawExceedsYield(uint256 requested, uint256 total);

   • Exploit Scenario

     N/A.

   • Recommendations

     Remove the unused custom error or use it when the withdraw exceeds the yield.

   • Results

     Resolved in commit 90291c1.

     The unused custom error was removed.

3. Typos

   Severity	Informational
   File	src/vaults/ImpactVault.sol#L136;
   Commit	dc3d7e3;
   Status	Resolved in commit 90291c1;

   • Description

     There’s a typo in line 136.

     * @notice Withdraws underlying asset by converting equivlaent value in

   • Exploit Scenario

     N/A.

   • Recommendations

     Correct the line specified to the following:

     * @notice Withdraws underlying asset by converting equivalent value in

   • Results

     Resolved in commit 90291c1.

     Typo was corrected.

4. Missing fields in event

   Severity	Informational
   File	src/vaults/SpiralsCeloImpactVault.sol#L40;
   Commit	dc3d7e3;
   Status	Resolved in commit 90291c1;

   • Description

     Some important fields are missing in event DependenciesUpdated.

     Event DependenciesUpdated contains the information of two variables:

     event DependenciesUpdated(
           address indexed manager,
           address indexed registry
       );

     But function setDependencies() updates with the following three parameters:

     function setDependencies(
           address _stCeloTokenAddress,
           address _stCeloManagerAddress,
           address _celoRegistryAddress
       )

     The Celo registry address should also be included to the event field

   • Exploit Scenario

     N/A.

   • Recommendations

     Add missing field to the DependenciesUpdated event.

   • Results

     Resolved in commit 90291c1.

     The missing variable was added to the event.

5. hasOutstandingWithdrawal() should also check if the value is non-zero

   Severity	Informational
   File	src/vaults/SpiralsCeloImpactVault.sol#L209;
   Commit	dc3d7e3;
   Status	Resolved in commit 7d48c3c;

   • Description

     Function _withdraw() calls hasOutstandingWithdrawal() to check if the current user has an outstanding withdrawal. Besides from timestamp, the value should also be checked otherwise user who input zero value to withdraw will trigger withdraw lock automatically.

     function hasOutstandingWithdrawal(address _address) public view returns (bool) {
           return withdrawals[_address].timestamp != 0;
       }

     When users try to withdraw funds from Celo impact vault, _withdraw() will called. It updates users withdraw timestamp with an unlocking period when user has outstanding withdraw and the withdraw value withe the value passed in by user.

     Whether the user qualifies the withdraw only depends on the timestamp in function hasOutstandingWithdrawal(). However, user might accidentally pass in a zero value and it will get user trapped in an unlocking waiting period and user can only withdraw zero value after that.

   • Exploit Scenario

     N/A.

   • Recommendations

     Also check if withdrawInfo.value is non-zero in hasOutstandingWithdrawal() function to prevent users accidentally lock themselves for an unlocking period of time.

   • Results

     Resolved in commit 7d48c3c.

     A non-zero value check is added to _beforeWithdraw().

6. Redundant indexed in events

   Severity	Informational
   File	ImpactVaultManager.sol#L33, L34; SpiralsCeloImpactVault.sol#L38, L39; SpiralsCUSDImpactVault#L24
   Commit	dc3d7e3;
   Status	Acknowledged;

   • Description

     The parameter amount of the events is indexed. A numerical value usually should not be indexed as it won’t accelerate searching.

     event Receive(address indexed sender, uint256 indexed amount);
       event Claim(address indexed receiver, uint256 indexed amount);

   • Exploit Scenario

     N/A.

   • Recommendations

     Remove the indexed keyword from the numerical values in the events:

     event Receive(address indexed sender, uint256 amount);
       event Claim(address indexed receiver, uint256 amount);

   • Results

     Acknowledged.

7. Deposit only when the amount is not zero

   Severity	Informational
   File	src/staking/migration/SpiralsStakingMigration.sol#L42; src/staking/migration/SpiralsCUSDVaultMigration.sol#L39;
   Commit	d7debb4;
   Status	Resolved in commit 6dcce64;

   • Description

     For function transferAssetsToImpactVaultForAddress(), a condition check can be added to make sure it only deposits into the new impact vault when the amount is not zero.

   • Exploit Scenario

     N/A.

   • Recommendations

     Add the following checks:

     function transferAssetsToImpactVaultForAddress(address _address, ImpactVault _vault) public onlyOwner whenPaused {
           /// ...
       
           if(amount != 0) {
               celoVault.depositCelo{value: amount}(_address);
           }
       
           /// ...
       }

     function transferAssetsToImpactVaultForAddress(address _address, ImpactVault _vault) public onlyOwner whenPaused {
           /// ...
       
           if(cUSDAmount != 0) {
               cUSDVault.deposit(cUSDAmount, _address);
           }
               
           /// ...
        }

   • Results

     Resolved in commit 6dcce64.

     Require statement was added to check if the deposit amount is zero.

• The deployer of these contracts should define the principal asset, yield asset, and ImpactVaultManager address.

• The ImpactVaultManager.sol contract can withdraw the yield accumulated by the asset.

The deployer of the contract has control over the following functionality:

• Set the governance token that will be associated with the yield generated by the ImpactValut contracts.

• Register new ImpactVault contracts.

• Remove a registered ImpactVault contract.