Spectra - ZanyBonzy's results

A permissionless interest rate derivatives protocol on Ethereum.

General Information

Platform: Code4rena

Start Date: 23/02/2024

Pot Size: $36,500 USDC

Total HM: 2

Participants: 39

Period: 7 days

Judge: Dravee

Id: 338

League: ETH

Spectra

Findings Distribution

Researcher Performance

Rank: 5/39

Findings: 2

Award: $399.40

🌟 Selected for report: 0

🚀 Solo Findings: 0

Awards

80.5733 USDC - $80.57

Labels

bug
2 (Med Risk)
high quality report
partial-75
sponsor confirmed
:robot:_33_group
duplicate-210

External Links

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L460 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L466 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L493 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L229-L275 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L278-L326

Vulnerability details

Impact

  1. maxWithdraw, maxWithdrawIBT, previewWithdrawIBT and previewWithdraw must not revert, but are pausable, causing them to revert when the contract is paused.

  2. The redeem functions must be callable by the owner or a user approved by the owner

  3. The withdraw functions must be callable by the owner or a user approved by the owner

Proof of Concept

The PrincipalToken contract aims to be fully EIP5095 compliant, but isn't fully compliant, breaking composability.

  1. maxWithdraw, maxWithdrawIBT, previewWithdrawIBT and previewWithdraw must not revert

These functions hold a whenNotPaused modifier, causing them to revert when the contract is paused.

/** @dev See {IPrincipalToken-maxWithdraw}. */ function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) { return convertToUnderlying(_maxBurnable(owner)); } /** @dev See {IPrincipalToken-maxWithdrawIBT}. */ function maxWithdrawIBT(address owner) public view override whenNotPaused returns (uint256) { return _convertSharesToIBTs(_maxBurnable(owner), false); }

According to the ERC5095 spec,

maxWithdraw

Maximum amount of the underlying asset that can be redeemed from the holder principal token balance, through a withdraw call.

...

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

MUST NOT revert.

function previewWithdraw( uint256 assets ) external view override whenNotPaused returns (uint256) { uint256 ibts = IERC4626(ibt).previewWithdraw(assets); return previewWithdrawIBT(ibts); } /** @dev See {IPrincipalToken-previewWithdrawIBT}. */ function previewWithdrawIBT(uint256 ibts) public view override whenNotPaused returns (uint256) { return _convertIBTsToShares(ibts, true); }

According to the ERC5095 spec,

previewWithdraw

Allows an on-chain or off-chain user to simulate the effects of their withdrawal at the current block, given current on-chain conditions.

...

MUST NOT revert due to principal token contract specific user/global limits. MAY revert due to other conditions that would also cause withdraw to revert.


  1. The redeem functions must be callable by the owner or a user approved by the owner
function redeem( uint256 shares, address receiver, address owner ) public override returns (uint256 assets) { _beforeRedeem(shares, owner); ... } /** @dev See {IPrincipalToken-redeem}. */ function redeem( uint256 shares, address receiver, address owner, uint256 minAssets ) external override returns (uint256 assets) { assets = redeem(shares, receiver, owner); ... } /** @dev See {IPrincipalToken-redeemForIBT}. */ function redeemForIBT( uint256 shares, address receiver, address owner ) public override returns (uint256 ibts) { _beforeRedeem(shares, owner); ... } /** @dev See {IPrincipalToken-redeemForIBT}. */ function redeemForIBT( uint256 shares, address receiver, address owner, uint256 minIbts ) external override returns (uint256 ibts) { ibts = redeemForIBT(shares, receiver, owner); ... }

These functions call the _beforeRedeem function, which burns the shares from the owner. Note that the function reverts if msg.sender is not the owner.

function _beforeRedeem(uint256 _shares, address _owner) internal nonReentrant whenNotPaused { if (_owner != msg.sender) { //@note revert UnauthorizedCaller(); } ... }

Hence, an approved user cannot redeem on behalf of the owner.

But according to the spec,

redeem

At or after maturity, burns exactly principalAmount of Principal Tokens from from and sends underlyingAmount of underlying tokens to to.

...

MUST support a redeem flow where the Principal Tokens are burned from holder directly where holder is msg.sender or msg.sender has EIP-20 approval over the principal tokens of holder. MAY support an additional flow in which the principal tokens are transferred to the Principal Token contract before the redeem execution, and are accounted for during redeem.


  1. The withdraw functions must be callable by the owner or a user approved by the owner

_beforeWithdraw reverts but withdrawIBT and withdraw should be performable by owner or approved owner

function withdraw( uint256 assets, address receiver, address owner ) public override returns (uint256 shares) { _beforeWithdraw(assets, owner); (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false); uint256 ibts = IERC4626(ibt).withdraw(assets, receiver, address(this)); shares = _withdrawShares(ibts, receiver, owner, _ptRate, _ibtRate); } /** @dev See {IPrincipalToken-withdraw}. */ function withdraw( uint256 assets, address receiver, address owner, uint256 maxShares ) external override returns (uint256 shares) { shares = withdraw(assets, receiver, owner); ... } /** @dev See {IPrincipalToken-withdrawIBT}. */ function withdrawIBT( uint256 ibts, address receiver, address owner ) public override returns (uint256 shares) { _beforeWithdraw(IERC4626(ibt).previewRedeem(ibts), owner); (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false); shares = _withdrawShares(ibts, receiver, owner, _ptRate, _ibtRate); ... } /** @dev See {IPrincipalToken-withdrawIBT}. */ function withdrawIBT( uint256 ibts, address receiver, address owner, uint256 maxShares ) external override returns (uint256 shares) { shares = withdrawIBT(ibts, receiver, owner); ... }

These functions all call the _beforeWithdraw function, which burns the shares from the owner. Note that the function reverts if msg.sender is not the owner.

function _beforeWithdraw(uint256 _assets, address _owner) internal whenNotPaused nonReentrant { if (_owner != msg.sender) { //@note revert UnauthorizedCaller(); ...

Hence, an approved user cannot withdraw on behalf of the owner.

But according to the spec,

withdraw

Burns principalAmount from holder and sends exactly underlyingAmount of underlying tokens to receiver.

...

MUST support a withdraw flow where the principal tokens are burned from holder directly where holder is msg.sender or msg.sender has EIP-20 approval over the principal tokens of holder. MAY support an additional flow in which the principal tokens are transferred to the principal token contract before the withdraw execution, and are accounted for during withdraw.

Tools Used

Manual code review

  1. maxWithdraw, maxWithdrawIBT, previewWithdrawIBT and previewWithdraw must not revert

Remove the whenNotPaused modifier and for the maxWithdraw/maxWithdrawIBT function, return 0 when paused.

  1. The redeem functions must be callable by the owner or a user approved by the owner

Refactor the check for unauthorized caller in the _beforeRedeem function to include approved holders. Include also a check that allowance => shares to redeem.

  1. The withdraw functions must be callable by the owner or a user approved by the owner

Refactor the check for unauthorized caller in the _beforeWithdraw function to include approved holders. Include also a check that allowance => shares to redeem.

Assessed type

Other

#0 - c4-pre-sort

2024-03-03T09:18:40Z

gzeon-c4 marked the issue as primary issue

#1 - c4-pre-sort

2024-03-03T09:25:22Z

gzeon-c4 marked the issue as high quality report

#2 - c4-sponsor

2024-03-06T10:40:27Z

yanisepfl (sponsor) confirmed

#3 - yanisepfl

2024-03-06T10:40:47Z

This is a good catch, thanks!

#4 - c4-judge

2024-03-11T00:33:50Z

JustDravee marked the issue as satisfactory

#5 - c4-judge

2024-03-11T00:33:57Z

JustDravee marked the issue as partial-75

#6 - c4-judge

2024-03-11T00:34:19Z

JustDravee marked issue #210 as primary and marked this issue as a duplicate of 210

#7 - ZanyBonzy

2024-03-12T08:55:27Z

Hi @JustDravee , is there a reason why this is marked as partial 75, where as some other reports like 212, 61, etc that didn't mention the full scope like the issue selected for report does much are marked as satisfactory. Another point of contention is that the issue selected for report doesn't mention any of the non-compliances of the previewWithdraw functions. I'd appreciate a second look at this, thanks.

#8 - JustDravee

2024-03-14T06:16:12Z

Hi @ZanyBonzy ,

Basically, wardens more or less grouped a certain amount of lack of compliance and I went with numbers.

I counted yours as having 2 categories of them: those related to maxWithdraw() reverting due to the pause (encompassing previewWithdraw() even if not mentioned because I'd expect Spectra to mirror the new logic "obviously"), and those related to the lack of user approved by the owner capabilities.

But here, yours failed to mentioned that maxRedeem isn't returning 0 if redeem is disabled. Hence 75%. Issue 212 mentioned it, but forgot the lack of approved users capabilities, so it got 75% too. Thank you for mentioning 61, as you could see in the history, it had 25%, but somehow the label got removed (not intentionally, I'll make another pass to see if this happened elsewhere). I applied 25% again, because it only had the "maxRedeem" bug.

Again, your report is indeed of high quality, and it wasn't easy finding a fair solution here, given the different mixes of different lack of compliance issues.

#9 - ZanyBonzy

2024-03-14T06:47:52Z

Understood, thanks)

Findings Information

🌟 Selected for report: hunter_w3b

Also found by: 0xbrett8571, DarkTower, Myd, ZanyBonzy, aariiif

Labels

analysis-advanced
grade-a
sufficient quality report
A-06

Awards

318.8332 USDC - $318.83

External Links

Advanced Analysis Report for Spectra Finance

Audit approach

Brief Overview

Scope and Architecture Overview

Codebase Overview

Centralization Risks

Systemic Risks

Recommendations

Conclusions

Resources


1. Audit approach

Phase 1: Protocol Familiarization:The review process commenced with an exploration of various information sources such as the readMe, website, technical documentation, and Discord chats. This was followed by an initial assessment of the in-scope contracts, tests, and deployment scripts.

Phase 2: Deep Contract Review: A meticulous, line-by-line review of the contracts was conducted, following the expected interaction flow.

Phase 3: Issue Discussion and Resolution: Potential risk areas identified during the review were discussed with the developers to clarify whether they were genuine issues or intentional features of the protocol.

Phase 4: Reporting: Audit reports were finalized based on the findings.


2. Brief Overview

  • Spectra is a DeFi protocol that specializes in interest rate derivatives, operating on a permissionless basis. The basic building block in Spectra is yield tokenisation, which is achieved by enabling users to separate the yield from the principal asset of an Interest Bearing Token (IBT).
  • When a user deposits an IBT into Spectra, they a principal token (PT) and a yield token (YT) that represents the pincipal asset, while the YT represents the yield generated by the IBT. This mechanism allows holders of the YT for a specific IBT to claim the yield accrued by the corresponding deposited IBTs during the time they hold the YT.

3. Scope and Architecture Overview

<h3 align="center"> <b>Overall architecture</b></h3> <p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/031079db-f2c1-4937-90db-309e86ba7992" alt="Contract Architecture"> </p> <h3 align="center"> <b>Contract interactions graph</b></h3> <p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/85565f26-226b-4957-aaf8-161786b5c69a" alt="Contract Architecture"> </p>

Tokens

PrincipalToken.sol
  • The Principal Token is the core protocol contract that acts as a vault, allowing users to deposit IBTs or their underlying tokens and receive the Principal and Yield tokens in return, tokenizing their yield in the process. The contract is expected to be EIP 5095, and 2612 compliant. To assist the contract in its functionalities is the PrincipalTokenUtils contract.

Asset deposit - Users can call the deposit and depositIBT functions to deposit a specified amount of underlying assets or the IBT tokens in the vault. These functions calculate and mint a specified amount of PT and YT shares for the users. It also updates the user's yield before minting. Important to note that to protect from slippage issues, users have the option to specify the minimum amount of shares they would like to receive.

Asset redemption or withdrawal - Depending on if users decide to specify the amount of shares to burn, or the amount of assets they would like to receive, the users can call the redeem, redeemForIBT or the withdraw, withdrawIBT functions respectively. These functions burn the PT and YT shares before YT expiry, or only the PT after the YT expiry, to provide the users with their assets. Users also have the option to enter the minimum assets to redeem or maximum shares to burn to protect from slippage.

Yield claim - As user's holding the YT shares, they're entitled to earn yields on their assets. By calling the claimYield and claimYieldInIBT functions, users can claim these yields. The function at first, updates the user's yields to match the latest timeframe, upon which the yield is restored to the user.

Flashloan - The principal token contracts allows for users to take flashloans on behalf of a receiver. The function aims to be EIP 3156 compliant. Receivers of this flashloan must implement the IERC3156FlashBorrower interface with enough tokens to refund the loan and fees.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/ab0b79f1-c252-419e-b1c7-1e46ca996363" alt="PrincipalToken.sol"> <br> sLOC - 649 </p>
YieldToken.sol
  • The YieldToken is an ERC20 & ERC2612 compliant token which helps in the tracking of users' yield. It represents the yield that is accrued by the IBT. It is created when the IBT or its underlying asset is deposited into the PrincipalToken contract.

Yield token minting and burning - When an IBT or its underlying asset is deposited into the contract, the corresponding yield token is minted to the receiver through the mint function. All calls to this function must originate from the PrincipalToken contract to prevent minting attacks. Upon redemption or withdrawal before yield token expiry, the burnWithoutUpdate function is called to burn the user's tokens without updating the user's yield. User's also have the option of directly burning their tokens by calling the burn function which updates the user's yield before burning the tokens.

Yield token transfer - The transfer and transferFrom methods hold the beforeYtTransfer hook which updates the transfer parties' yield before the token transfer is executed. Users call these functions to share tokens and also update their yields in the process.

balanceOf vs actualBalanceOf discrepancy - The contract implements the balanceOf and actualBalanceOf functions, the latter which displays the balance of an account before yield token expiry, otherwise it returns 0. The actualBalanceOf function displays the balance of an account without considering if the yield token has expired or not.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/91e90d4a-f9fe-4a52-9977-60e07aa00ce5" alt="YieldToken.sol"> <br> sLOC - 73 </p>

Proxy contracts

These are implementations of openzeppelin proxy contracts which have however been modified to use the AccessManaged contract for access control in place of Ownable. The contracts here are the AMBeacon, AMProxyAdmin and AMTransparentUpgradeableProxy contracts.

AMTransparentUpgradeableProxy.sol
  • This contract implements a proxy that is upgradeable through an predefined AMProxyAdmin contract. The admin uses this contract to initialize an upgradeable proxy managed by the AMProxyAdmin.
<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/f6dff8ec-8742-4df6-9bd4-57e53d0163d1" alt="AMTransparentUpgradeableProxy.sol"> <br> sLOC - 42 </p>
AMProxyAdmin.sol
  • This acts as the auxialiary admin contract for the AMTransparentUpgradeableProxy. Through this contract the admin can upgrade proxies to implementations and at the same time, call functions on the new implementation.
<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/967547be-2d1f-41ba-ab21-ff04dc8db146" alt="AMProxyAdmin.sol"> <br> sLOC - 14 </p>
AMBeacon.sol
  • The AMBeacon contract is used in together with other beacon proxy contracts to determine their implementation contract from which the get all their delegated function calls. Here, the admin can upgrade the beacon to a new contract implementation.
<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/a7969815-e6c5-4cb1-9581-4daa5e3715b4" alt="AMBeacon.sol"> <br> sLOC - 24 </p>

Utilities

The utility contracts are the various libraries that the protocol depends on to perform various functions.

PrincipalTokenUtil.sol
  • The contract is a library extension of the PrincipalToken contract that executes various calculation functions. Here, yield, yield fees, flashloan fees, tokenization fees are calculated, asset to shares and share to asset conversions are made. This is a very important aspect of the protocol as wrong calculations here will severly affect flow of funds into and out of the principal token contract.
<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/7c9a545c-72b7-4a6c-a766-a4df09768503" alt="PrincipalTokenUtil.sol"> <br> sLOC - 142 </p>
RayMath.sol
  • The RayMath contract is in charge of token decimal conversions. It aims to provide a level decimal representation for all token by matching them all to a ray (27 decimals).
<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/984e89a6-98d2-4310-8f1f-3bb5ebd37f17" alt="RayMath.sol"> <br> sLOC - 32 </p>

4. Codebase Overview

Audit Information - For the purpose of this audit, the Spectra Finance codebase comprises of seven smart contracts totaling 976 SLoC. It holds fifteen external imports, five external calls, two seperate interface and struct definitions. Its core design principle is inheritance, enabling efficient and flexible integration. It is scheduled to be deployed to the Ethereum, Polygon, Arbitrum chains, giving each user the freedom to transact in and across all platforms. It operates independently of oracles and sidechains.

Documentation and NatSpec - The codebase provides important resources. The provided documentation is tight, up to date, although misses information about certain utility contracts. They provide a good technical breakdown of the contracts, functions and their intentions. The contracts are also well commented, not strictly to NatSpec but each function was well explained. It made the audit process easier.

Error handling and Event Emission - Custom errors were defined and in use in the codebase, marking a point of gas efficiency. Events are well handled, emitted for important parameter changes, although in some cases, they seemed to not follow the checks-effects-interaction patterns and were emitted in loops.

Testability - Test coverage is about 96% which is very good. The implemented tests are mosty unit tests which tests the basic functionalities of the functions and helped with basic bugs. A bit of fuzz and invariant tests were also provided.

Token Support - The protocol works with the various interest bearing ERC20 tokens except for ERC777 tokens. In return, users receive a deployed principal and yield token. The contracts implementations is mostly suited for most token types as the safeERC20 functions are implemented, except for fee-on-transfer tokens.

Attack Vectors - For the protcool, the main attack vectors include gaming yields, stealing other user's rewards, griefing, inflation attacks. Other points to consider are the EIP compliances especially as the contracts aim to interact with broader defi protocols and vaults, correct function implementations and so on.


5. Centralization Risks

Like any protocol that incorporates admin functions, the actions of a malicious admin can leave the protcol vulnerable. Some of them include:

  • Maliciously griefing users by pausing the protocol and renouncing role. This issue is further excarcebated because withdrawals and redemptions can also be paused.
  • Automatically locking yield tokens by setting duration to 0 upon principal token initialization, such that yield tokens expire as on contract initialization.
  • Setting malicious implementations to rug users.

6. Systemic Risks

  • User activity is very important as the protcol will not really function if users are not very active in the protcol.
  • Lack of a sweep function in the contracts can lead to loss of tokens that cannot be recovered.
  • Malicious users looking to grief other users or steal rewards.
  • Issues with external dependencies, solidity version bugs and open zeppelin contracts.
  • Issues from new upgrades, new protocol implementations.
  • Vulnerabilities from other smart contracts, which at best may be contained in the erring contracts, at worst affect the whole protocol.

7. Recommendations

  • The codebase should be sanitized, and the comments should also be brought up to date to conform with NatSpec. Some of the functions in the PrincipalTokenUtil contract reference a non-existent part of the documentation, these need to be updated.

  • Some tokens have variable balances or inplement fees on transfer, balance checks before and after these transfers can help track the balance changes so as to prevent accounting issues.

  • Allowing anyone to flashloan on behalf of a receiver can be abused to drain unsuspecting user's of their balance. Especially if they had previously given the principal token approvals to spend their tokens.

  • The Principal token contract should also be fully brought up to full compliance by fixing a number of the protential issues highlighted, like removing the whenNotPaused modifiers on the maxWithraw and maxRedeem functions, allowing users approved by a token holder to withdraw/redeem on their behalf and so on.

  • About pausing, it's not advisable to pause redemptions or withdrawals as user's funds can easily be locked by rouge/compromised admins. Also, it inspires a lot of confidence in users, knowing they can access their funds at any time they wish.

  • Two step variable and address updates should be implemented. Most of the setter functions implement the changes almost immediately which can be jarring for the contracts/users. Consider introducing timelocks. These fixes can help protect from mistakes and unexepected behaviour.

  • The PrincipalToken contract aims to be upgradeable, but sill uses certain unupagradeable openzeppelin versions, and has no storage gaps. This should be improved upon.

  • Testing should be improved, including more invariant and fuzzing tests;

  • Solidity and OpenZeppelin contract versions should be updated to the latest as they provide lots of benefits in security and optimization;


8. Conclusions

In general, the codebase is pretty complex, due to its various integrations, albeit, of small size. It is however very well-designed. As is the reason for the audit, the identified risks need to be fixed. Recommended measures should be implemented to protect the protocol from potential attacks. Timely audits and sanitizations should be conducted to keep the codebase fresh and up to date with evolving security times.


9. Resources

Time spent:

036 hours

#0 - c4-pre-sort

2024-03-03T14:06:53Z

gzeon-c4 marked the issue as sufficient quality report

#1 - jeanchambras

2024-03-09T19:23:21Z

The report is good, straightforward, and includes a relevant risk assessment.

#2 - c4-judge

2024-03-11T00:51:26Z

JustDravee marked the issue as grade-a

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter