Spectra - sl1's results

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L229-L237 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L278-L287 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L493-L495 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L488-L490 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L460-L462

Vulnerability details

Bug description

Principal Token contract is an ERC5095 vault that allows users to tokenize their yield in a permissionless manner.

Important The contest page explicitly states that PrincipalToken must conform with the EIP-5095.

The following specification of redeem() is taken directly from the EIP-5095:

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.

The following specification of withdraw() is taken directly from the EIP-5095:

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.

Both these functions should allow an approved user to redeem or withdraw approved Principal tokens, but currently, that's impossible. All redeem functions internally call _beforeRedeem(), which has this check in place:

if (_owner != msg.sender) {
            revert UnauthorizedCaller();
        }

The same can be said about every withdraw function. All of them call _beforeWithdraw(), which has the same check in place.

As per EIP-5095 both convertToUnderlying() and convertToPrincipal() functions MUST NOT revert unless due to integer overflow caused by an unreasonably large input. In case of convertToUnderlying() function, it calls _convertSharesToIBTs(), which can revert with RateError when ibtRate is 0. In case of convertToPrincipal() function, it calls _convertIBTsToShares(), which can revert with RateError when ptRate is 0.

As per EIP-5095 maxWithdraw() must not revert, but it can revert because of whenNotPausedModifier or because of the RateError mentioned above.

Impact

PrincipalToken contract does not respect the EIP standards it claims to follow. This breaks compatibility and violates invariants stated in the README.

Recommended Mitigation

Ensure that PrincipalToken is EIP-compliant. Alternatively, document that some functions deviate from the ERC5095 specification.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L369-L374 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L329-L337

Vulnerability details

Bug description

claimFees() function allows the fee collector set by the protocol to claim fees in IBT. claimFees()

function claimFees() external override returns (uint256 assets) {
        if (msg.sender != IRegistry(registry).getFeeCollector()) {
            revert UnauthorizedCaller();
        }
        uint256 ibts = unclaimedFeesInIBT;
        unclaimedFeesInIBT = 0;
        assets = IERC4626(ibt).redeem(ibts, msg.sender, address(this));
        emit FeeClaimed(msg.sender, ibts, assets);
    }

claimYield() allows users to claim their yield in IBT. claimYield()

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

As can be seen, both these functions redeem ibts from the ERC4626 vault, but they lack minAssets check. To protect users or feeCollector from receiving less assets for their amount of ibts, minAssets parameter should be used.

Impact

The entire amount of ibts can be lost.

Recommended Mitigation

claimYield()

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

claimFees()

- function claimFees() external override returns (uint256 assets) {
+ function claimFees(uint256 minAssets) external override returns (uint256 assets) {
        if (msg.sender != IRegistry(registry).getFeeCollector()) {
            revert UnauthorizedCaller();
        }
        uint256 ibts = unclaimedFeesInIBT;
        unclaimedFeesInIBT = 0;
        assets = IERC4626(ibt).redeem(ibts, msg.sender, address(this));
+       require(assets >= minAssets);
        emit FeeClaimed(msg.sender, ibts, assets);
    }

Assessed type

Other