Spectra - Giorgio's results

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/25603ac27c3488423a0739b66e784c01a3db7d75/src/tokens/PrincipalToken.sol#L484-L486 https://github.com/code-423n4/2024-02-spectra/blob/25603ac27c3488423a0739b66e784c01a3db7d75/src/tokens/PrincipalToken.sol#L867-L875

Vulnerability details

Vulnerability detail

The principalToken.sol contract claims to be ERC5095 comliant. The EIP 5095 standard wants the maxRedeem() function to return 0 if redemption are paused. But this is not the current case .

Impact

The contract fails to be ERC5095 compliant, which is a requirement stated in the contest page.

Proof of Concept

Here

    function maxRedeem(address owner) public view override returns (uint256) {
        return _maxBurnable(owner);
    }

We can see that the redeem function makes an internal call to the _maxBurnable internal function

    function _maxBurnable(address _user) internal view returns (uint256 maxBurnable) {
        if (block.timestamp >= expiry) {
            maxBurnable = balanceOf(_user);
        } else {
            uint256 ptBalance = balanceOf(_user);
            uint256 ytBalance = IYieldToken(yt).balanceOf(_user);
            maxBurnable = (ptBalance > ytBalance) ? ytBalance : ptBalance;
        }
    }

It is here flagrant that the results won't be altered if the redemptions are paused.

Tools Used

EIP-5095 page

MUST return the maximum amount of principal tokens that could be transferred from holder through redeem and not cause a revert

Recommended Mitigation Steps

return 0 if the redemption are paused. As such:

    function maxRedeem(address owner) public view override returns (uint256 result) {
        if(paused()) result = 0;
        result = _maxBurnable(owner);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/25603ac27c3488423a0739b66e784c01a3db7d75/src/tokens/PrincipalToken.sol#L461-L463

Vulnerability details

Vulnerability explanation

According to the almighty EIP-5095, the maxWithdraw() function "MUST return the maximum amount of underlying tokens that could be redeemed from holder through withdraw and not cause a revert". However instead of this the cheeky maxWithdraw() function in the contract will revert when paused because of the whenNotPaused modifier

Impact

The function reverts when the redemptions are paused, which is not in line with the requirements of the EIP-5095.

Proof of Concept

The maxWithdraw()

    function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
        return convertToUnderlying(_maxBurnable(owner)); //@note inclusive of fees
    }

reverts when the DAO decides to paused the contract. If the function is called while paused the function reverts.

    function _requireNotPaused() internal view virtual {
        if (paused()) {
 @>          revert EnforcedPause();
        }
    }

Tools Used

EIP-5095 docs

Recommended Mitigation Steps

If the contract happens to be paused, return 0 instead of revertin'.

   -- function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
     
   ++ function maxWithdraw(address owner) public view override returns (uint256) {
 ++    if(paused()) return 0;
       return convertToUnderlying(_maxBurnable(owner)); //@note inclusive of fees
    }

Assessed type

Other