Spectra - SBSecurity's results

Low Risk

Non-Critical

Low Risks

[L-01] Users with a 100% fee reduction can deposit on behalf of other people and cause Spectra to lose fees

Issue Description:

Fee reduction can be abused when a user who is granted 100% reduction aggregates funds and deposits them on behalf of other users, this will lead to loss of rewards for Spectra.

PrincipalTokenUtil.sol#L166

function _computeTokenizationFee(
    uint256 _amount,
    address _pt,
    address _registry
) internal view returns (uint256) {
    return
        _amount
            .mulDiv(IRegistry(_registry).getTokenizationFee(), FEE_DIVISOR, Math.Rounding.Ceil)
            .mulDiv(
                FEE_DIVISOR - IRegistry(_registry).getFeeReduction(_pt, msg.sender),
                FEE_DIVISOR,
                Math.Rounding.Ceil
            );
}

Recommendation:

Consider capping the max deposits for users with fee reduction.

[L-02] Only vaults that allow share transfers can be used

Issue Description:

ERC4626 vaults that doesn’t allow shares to be transferred will make the following functions useless:

• depositIBT
• redeemForIBT
• withdrawIBT
• claimYieldInIBT
• flashLoan

Especially inability to execute flash loans with the IBT shares greatly will limit the functionality of PrincipalToken.

Recommendation:

Given the decentralized nature of the Spectra protocol there is nothing that can be done in order to mitigate this issue. It is up to the deployers which ERC4626 vault they will decide to use or even deploy their own.

[L-03] PtRate cannot increase if IbtRate ≥ oldIbtRate

Issue Description:

When rates are updated, ptRate will only update (always decrease) only if ibtRate decreases.

function _getCurrentPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
    uint256 currentIBTRate = IERC4626(ibt).previewRedeem(ibtUnit).toRay(_assetDecimals);
    if (IERC4626(ibt).totalAssets() == 0 && IERC4626(ibt).totalSupply() != 0) {
        currentIBTRate = 0;
    }
    // NOTE - If ibtRate is still the same, ptRate will not change either
    uint256 currentPTRate = currentIBTRate < ibtRate
        ? ptRate.mulDiv(currentIBTRate, ibtRate, roundUpPTRate ? Math.Rounding.Ceil : Math.Rounding.Floor)
        : ptRate;
    return (currentPTRate, currentIBTRate);
}

_getCurrentPTandIBTRates

Because of the in the computeYield function, there is part that is unreachable.

function _computeYield(
    address _user,
    uint256 _userYieldIBT,
    uint256 _oldIBTRate,
    uint256 _ibtRate,
    uint256 _oldPTRate,
    uint256 _ptRate,
    address _yt
) external view returns (uint256) {
    if (_oldPTRate == _ptRate && _ibtRate == _oldIBTRate) {
        return _userYieldIBT;
    }
    uint256 newYieldInIBTRay;
    uint256 userYTBalanceInRay = IYieldToken(_yt).actualBalanceOf(_user).toRay(
        IYieldToken(_yt).decimals()
    );
    // ibtOfPT is the yield generated by each PT corresponding to the YTs that the user holds
    uint256 ibtOfPTInRay = userYTBalanceInRay.mulDiv(_oldPTRate, _oldIBTRate);
    if (_oldPTRate == _ptRate && _ibtRate > _oldIBTRate) {
        // only positive yield happened
        newYieldInIBTRay = ibtOfPTInRay.mulDiv(_ibtRate - _oldIBTRate, _ibtRate);
    } else {
        if (_oldPTRate > _ptRate) { 
            // PT depeg happened
            uint256 yieldInAssetRay;

This is unreachable because for ptRate to decrease, ibt Rate must also decrease.
----------------------------------------------
            if (_ibtRate >= _oldIBTRate) {
                // both negative and positive yield happened, more positive
                yieldInAssetRay =
                    _convertToAssetsWithRate(
                        userYTBalanceInRay,
                        _oldPTRate - _ptRate,
                        RayMath.RAY_UNIT,
                        Math.Rounding.Floor
                    ) +
                    _convertToAssetsWithRate(
                        ibtOfPTInRay,
                        _ibtRate - _oldIBTRate,
                        RayMath.RAY_UNIT,
                        Math.Rounding.Floor
                    );
------------------------------------------------
            } else {
                // either both negative and positive yield happened, more negative
                // or only negative yield happened
                uint256 actualNegativeYieldInAssetRay = _convertToAssetsWithRate(
                    userYTBalanceInRay,
                    _oldPTRate - _ptRate,
                    RayMath.RAY_UNIT,
                    Math.Rounding.Floor
                );
                uint256 expectedNegativeYieldInAssetRay = Math.ceilDiv(
                    ibtOfPTInRay * (_oldIBTRate - _ibtRate),
                    RayMath.RAY_UNIT
                );
                yieldInAssetRay = expectedNegativeYieldInAssetRay >
                    actualNegativeYieldInAssetRay
                    ? 0
                    : actualNegativeYieldInAssetRay - expectedNegativeYieldInAssetRay;
                yieldInAssetRay = yieldInAssetRay.fromRay(
                    IERC4626(IPrincipalToken(IYieldToken(_yt).getPT()).underlying()).decimals()
                ) < SAFETY_BOUND
                    ? 0
                    : yieldInAssetRay;
            }
            newYieldInIBTRay = _convertToSharesWithRate(
                yieldInAssetRay,
                _ibtRate,
                RayMath.RAY_UNIT,
                Math.Rounding.Floor
            );
        } else {
            // PT rate increased or did not depeg on IBT rate decrease
            revert IPrincipalToken.RateError();
        }
    }
    return _userYieldIBT + newYieldInIBTRay.fromRay(IERC20Metadata(_yt).decimals());
}

_computeYield

Recommendation:

Remove this part if the rate update logic remains as it is now.

Non-Critical

[N‑01] _computeYield should use ERC20Metadata instead of IERC4626

Issue Description:

When both PT and IBT rates are decreasing, _computeYield will enter the else statement where it will check if the expected and actual yields are more than the SAFETY_BOUND . The value checked against the invariant is converted to the decimals of the underlying token of IBT, but the wrong interface is used there. The underlying will be “cast” to the ERC4626 interface, but since ERC4626 inherits from ERC20, it will have decimals(). At all this only use the wrong interface, as the underlying is ERC20.

PrincipalTokenUtils.sol#L113

function _computeYield(
		address _user,
		uint256 _userYieldIBT,
		uint256 _oldIBTRate,
		uint256 _ibtRate,
		uint256 _oldPTRate,
		uint256 _ptRate,
		address _yt
) external view returns (uint256) {
      yieldInAssetRay = yieldInAssetRay.fromRay(
          IERC4626(IPrincipalToken(IYieldToken(_yt).getPT()).underlying()).decimals()
      ) < SAFETY_BOUND
          ? 0
          : yieldInAssetRay;
      }

Recommendation:

Instead of IERC4626 , consider using IERC20Metadata

function _computeYield(
		address _user,
		uint256 _userYieldIBT,
		uint256 _oldIBTRate,
		uint256 _ibtRate,
		uint256 _oldPTRate,
		uint256 _ptRate,
		address _yt
) external view returns (uint256) {
      yieldInAssetRay = yieldInAssetRay.fromRay(
-          IERC4626(IPrincipalToken(IYieldToken(_yt).getPT()).underlying()).decimals()
+          IERC20Metadata(IPrincipalToken(IYieldToken(_yt).getPT()).underlying()).decimals()
      ) < SAFETY_BOUND
          ? 0
          : yieldInAssetRay;
      }

[N‑02] flashLoan function contains a useless _token argument

Issue Description:

In PrincipalToken flash loans are possible only with IBT shares, since this is the only tokens that the contract is intended to have, but flashLoan function requires explicitly to provide token as an argument and then there is a check in the flashFee function which requires token to be equal to IBT.

PrincipalToken.sol#L618

function flashLoan(
    IERC3156FlashBorrower _receiver,
    address _token,
    uint256 _amount,
    bytes calldata _data
) external override returns (bool) {
    if (_amount > maxFlashLoan(_token)) revert FlashLoanExceedsMaxAmount();

    uint256 fee = flashFee(_token, _amount); //@audit check token
    _updateFees(fee); 

...More code
}

PrincipalToken.sol#L595

function flashFee(address _token, uint256 _amount) public view override returns (uint256) {
    if (_token != ibt) revert AddressError(); //@audit only IBT token
    return PrincipalTokenUtil._computeFlashloanFee(_amount, registry);
}

Recommendation:

function flashLoan(
    IERC3156FlashBorrower _receiver,
-   address _token,
    uint256 _amount,
    bytes calldata _data
) external override returns (bool) {
-   if (_amount > maxFlashLoan(_token)) revert FlashLoanExceedsMaxAmount();
+   if (_amount > maxFlashLoan(ibt)) revert FlashLoanExceedsMaxAmount();

-   uint256 fee = flashFee(_token, _amount);
+   uint256 fee = flashFee(_amount);
    _updateFees(fee); 
		// Initiate the flash loan by lending the requested IBT amount
    IERC20(ibt).safeTransfer(address(_receiver), _amount);

    // Execute the flash loan
-   if (_receiver.onFlashLoan(msg.sender, _token, _amount, fee, _data) != ON_FLASH_LOAN)
+   if (_receiver.onFlashLoan(msg.sender, ibt, _amount, fee, _data) != ON_FLASH_LOAN)
        revert FlashLoanCallbackFailed();

    // Repay the debt + fee
    IERC20(ibt).safeTransferFrom(address(_receiver), address(this), _amount + fee);

    return true;
}

function flashFee(address _token, uint256 _amount) public view override returns (uint256) {
-   if (_token != ibt) revert AddressError(); //@audit only IBT token
    return PrincipalTokenUtil._computeFlashloanFee(_amount, registry);
}

[N‑03] _ibtUnit is wrongly named in the convert functions

Issue Description:

Convert functions in PrincipalTokenUtils have confusing argument _ibtUnit which is passed in the _computeYield. We can observe that in all the places where _convertToSharesWithRate and _convertToAssetsWithRate are used RayMath.RAY_UNIT is passed as _ibtUnit. But the _ibtRate will be in the ibt decimals, not in RAY.

function _computeYield(
    address _user,
    uint256 _userYieldIBT,
    uint256 _oldIBTRate,
    uint256 _ibtRate,
    uint256 _oldPTRate,
    uint256 _ptRate,
    address _yt
) external view returns (uint256) {
...More code
        yieldInAssetRay =
        _convertToAssetsWithRate( //@audit here
            userYTBalanceInRay,
            _oldPTRate - _ptRate,
            RayMath.RAY_UNIT,
            Math.Rounding.Floor
        ) +
        _convertToAssetsWithRate( //@audit here
            ibtOfPTInRay,
            _ibtRate - _oldIBTRate,
            RayMath.RAY_UNIT,
            Math.Rounding.Floor
        );
    } else {
        uint256 actualNegativeYieldInAssetRay = _convertToAssetsWithRate( //@audit here
            userYTBalanceInRay,
            _oldPTRate - _ptRate,
            RayMath.RAY_UNIT,
            Math.Rounding.Floor
        );
		...More code
    }
    newYieldInIBTRay = _convertToSharesWithRate( //@audit here
        yieldInAssetRay,
        _ibtRate,
        RayMath.RAY_UNIT,
        Math.Rounding.Floor
    );
 } 
}

Recommendation:

Rename _ibtUnit to ray or something more intuitive to the reader.

[N‑04] Wrong event emissions

Issue Description:

1. _depositIBT has Mint event that uses msg.sender as from, in fact it is address(0)

PrincipalToken.sol#L767

emit Mint(msg.sender, _ptReceiver, shares);

1. _withdrawShares emits Redeem instead of Withdraw it can be confusing for the off-chain event readers.

PrincipalToken.sol#L797

emit Redeem(_owner, _receiver, shares);

1. _claimYield passes msg.sender as both owner and receiver to YieldClaimed, but in fact receiver can be other address:

PrincipalToken.sol#L857

emit YieldClaimed(msg.sender, msg.sender, yieldInIBT);

PrincipalToken.sol#L369-L382

  function claimYield(address _receiver) public override returns (uint256 yieldInAsset) {
      uint256 yieldInIBT = _claimYield();
      if (yieldInIBT != 0) {
          yieldInAsset = IERC4626(ibt).redeem(yieldInIBT, _receiver, address(this));
      }
  }

  /** @dev See {IPrincipalToken-claimYieldInIBT}. */
  function claimYieldInIBT(address _receiver) public override returns (uint256 yieldInIBT) {
      yieldInIBT = _claimYield();
      if (yieldInIBT != 0) {
          IERC20(ibt).safeTransfer(_receiver, yieldInIBT);
      }
  }

Recommendation:

1. Pass address(0) instead of msg.sender
2. Create Withdraw event, and use it instead of Redeem.
3. Move the event at the calling function and pass the proper receiver.

[N‑05] _getPTandIBTRates can be simplified

Issue Description:

_getPTandIBTRates contains logic whether PT has expired and returns if so, also there is a redundant else statement which add unnecessary code to the function:

PrincipalToken.sol#L924-L926

function _getPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
    if (ratesAtExpiryStored) {
        return (ptRate, ibtRate);
    } else {
        return _getCurrentPTandIBTRates(roundUpPTRate);
    }
}

Recommendation:

Remove the else statement, so when maturity hasn’t passed code will automatically return the current PT and IBT, the same as it entered the else.

function _getPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
    if (ratesAtExpiryStored) {
        return (ptRate, ibtRate);
    } 
-   else {
        return _getCurrentPTandIBTRates(roundUpPTRate);
-   }
}

[NC-06] Typos

Issue Description:

Comments for ptRate and ibtRate contains unnecessary ‘or’ which can be removed:

PrincipalToken.sol#L55-L56

uint256 private ptRate; // or PT price in asset (in Ray)
uint256 private ibtRate; // or IBT price in asset (in Ray)

Recommendation:

Modify the comments by removing the or.

uint256 private ptRate; // PT price in asset (in Ray)
uint256 private ibtRate; // IBT price in asset (in Ray)

Also private variables names can be more consistent with underscore before them:

  address private rewardsProxy;
  bool private ratesAtExpiryStored;
  address private ibt; // address of the Interest Bearing Token 4626 held by this PT vault
  address private _asset; // the asset of this PT vault (which is also the asset of the IBT 4626)
  address private yt; // YT corresponding to this PT, deployed at initialization
  uint256 private ibtUnit; // equal to one unit of the IBT held by this PT vault (10^decimals)
  uint256 private _ibtDecimals;
  uint256 private _assetDecimals;

  uint256 private ptRate; // or PT price in asset (in Ray)
  uint256 private ibtRate; // or IBT price in asset (in Ray)
  uint256 private unclaimedFeesInIBT; // unclaimed fees
  uint256 private totalFeesInIBT; // total fees
  uint256 private expiry; // date of maturity (set at initialization)
  uint256 private duration; // duration to maturity