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
Rank: 5/39
Findings: 2
Award: $399.40
🌟 Selected for report: 0
🚀 Solo Findings: 0
80.5733 USDC - $80.57
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
maxWithdraw
, maxWithdrawIBT
, previewWithdrawIBT
and previewWithdraw
must not revert, but are pausable, causing them to revert when the contract is paused.
The redeem
functions must be callable by the owner or a user approved by the owner
The withdraw
functions must be callable by the owner or a user approved by the owner
The PrincipalToken contract aims to be fully EIP5095 compliant, but isn't fully compliant, breaking composability.
maxWithdraw
, maxWithdrawIBT
, previewWithdrawIBT
and previewWithdraw
must not revertThese 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.
redeem
functions must be callable by the owner or a user approved by the ownerfunction 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.
_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.
Manual code review
maxWithdraw
, maxWithdrawIBT
, previewWithdrawIBT
and previewWithdraw
must not revertRemove the whenNotPaused
modifier and for the maxWithdraw
/maxWithdrawIBT
function, return 0 when paused.
redeem
functions must be callable by the owner or a user approved by the ownerRefactor the check for unauthorized caller in the _beforeRedeem
function to include approved holders. Include also a check that allowance => shares to redeem.
withdraw
functions must be callable by the owner or a user approved by the ownerRefactor the check for unauthorized caller in the _beforeWithdraw
function to include approved holders. Include also a check that allowance => shares to redeem.
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)
🌟 Selected for report: hunter_w3b
Also found by: 0xbrett8571, DarkTower, Myd, ZanyBonzy, aariiif
318.8332 USDC - $318.83
Scope and Architecture Overview
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.
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.
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.
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.
AMProxyAdmin
contract. The admin uses this contract to initialize an upgradeable proxy managed by the AMProxyAdmin
.AMTransparentUpgradeableProxy
. Through this contract the admin can upgrade proxies to implementations and at the same time, call functions on the new implementation.The utility contracts are the various libraries that the protocol depends on to perform various functions.
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.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.
Like any protocol that incorporates admin functions, the actions of a malicious admin can leave the protcol vulnerable. Some of them include:
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;
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.
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