Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NES-294] [Fix] Slowmist remediations update #122

Merged
merged 4 commits into from
Dec 10, 2024
Merged

[NES-294] [Fix] Slowmist remediations update #122

merged 4 commits into from
Dec 10, 2024

Conversation

ungaro
Copy link
Contributor

@ungaro ungaro commented Dec 10, 2024

What's new in this PR?

  1. In PR120, the mint function of ComponentToken contract uses ERC4626's _deposit function for token deposits. However, it's important to note that the _deposit function in ERC4626 already mints shares to users, while the mint function in ComponentToken also uses _mint to mint shares again. This results in users receiving double shares.
function mint(
    uint256 shares,
    address receiver,
    address controller
) public virtual nonReentrant returns (uint256 assets) {
    ...
    } else {
        assets = previewMint(shares);
        _deposit(msg.sender, receiver, assets, shares);
    }
    _mint(receiver, shares);
    ...
}
  1. In PR120, during asynchronous minting, the mint function of ComponentToken contract doesn't verify if the user-provided shares amount matches the recorded amount in sharesDepositRequest. This could result in users receiving fewer shares than their deducted async deposit balance.
function mint(
    uint256 shares,
    address receiver,
    address controller
) public virtual nonReentrant returns (uint256 assets) {
    ...
    if ($.asyncDeposit) {
        if ($.sharesDepositRequest[controller] < shares) {
            revert InsufficientRequestBalance(controller, shares, 1);
        }
        // Use the pre-calculated assets amount from when deposit was notified
        assets = $.claimableDepositRequest[controller];
        $.claimableDepositRequest[controller] = 0;
        $.sharesDepositRequest[controller] = 0;
    } else {
        ...
    }
    _mint(receiver, shares);
    ...
}
  1. In PR120, the withdraw function of ComponentToken contract incorrectly checks shares at the beginning, when it should be checking the user-provided assets amount.
function withdraw(
    uint256 assets,
    address receiver,
    address controller
) public virtual override(ERC4626Upgradeable, IERC7540) nonReentrant returns (uint256 shares) {
    if (shares == 0) {
        revert ZeroAmount();
    }
    ...
}
  1. In PR120, during asynchronous withdrawal, the withdraw function doesn't verify if the user-provided assets amount matches the recorded amount in assetsRedeemRequest. This could result in the contract transferring fewer assets to users than the recorded assetsRedeemRequest amount.
function withdraw(
    uint256 assets,
    address receiver,
    address controller
) public virtual override(ERC4626Upgradeable, IERC7540) nonReentrant returns (uint256 shares) {
    ...
    if ($.asyncRedeem) {
        // Use the pre-calculated assets amount from when redeem was notified
        if ($.assetsRedeemRequest[controller] < assets) {
            revert InsufficientRequestBalance(controller, assets, 3);
        }
        shares = $.claimableRedeemRequest[controller];
        $.claimableRedeemRequest[controller] = 0;
        $.assetsRedeemRequest[controller] = 0;
    } ...
}
  1. In PR120, after asynchronous withdrawal, the withdraw function of ComponentToken contract fails to implement the functionality to transfer assets tokens to the receiver, preventing users from recovering funds through async withdrawal.

  2. In PR120, the withdraw function of ComponentToken contract performs a _burn operation after user withdrawal. However, for async withdrawals, the requestRedeem function has already burned the user's shares, and for sync withdrawals, the _withdraw function has also burned the user's shares. This leads to double-burning of user shares.

   function withdraw(
        uint256 assets,
        address receiver,
        address controller
    ) public virtual override(ERC4626Upgradeable, IERC7540) nonReentrant returns (uint256 shares) {
        ...
        _burn(msg.sender, shares);
        emit Withdraw(msg.sender, receiver, msg.sender, assets, shares);
        return shares;
    }
  1. In PR112, the function deposit(uint256 assets, address receiver, address controller) doesn't need to override the ERC4626Upgradeable contract, as this function doesn't exist in ERC4626Upgradeable.
    function deposit(
        ...
    ) public override(ComponentToken, IComponentToken, ERC4626Upgradeable) returns (uint256 shares) {
        ...
    }

@ungaro ungaro requested a review from eyqs December 10, 2024 21:02
@ungaro ungaro marked this pull request as ready for review December 10, 2024 21:03
Copy link
Member

@eyqs eyqs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nest/src/ComponentToken.sol Show resolved Hide resolved
@ungaro ungaro merged commit bf6895b into main Dec 10, 2024
1 check passed
@ungaro ungaro deleted the alp/NES-294 branch December 10, 2024 21:45
ungaro added a commit that referenced this pull request Dec 12, 2024
* slowmist review update

* fix mint function

* double check functions, formatting

* early exit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants