Neutrl Contracts

Summary

FULL_RESTRICTED_STAKER_ROLE role users are not allowed to withdraw funds normally. However it was observed that if user was backlisted post cooldownAssets/cooldownShares call then user can withdraw funds even though they are blacklisted.

Finding Description

1. User A stakes amount 100 NUSD and say gets 100 sNUSD
2. After sometime User wants to withdraw funds so User calls cooldownAssets with amount 100 NUSD
3. Admin is suspicious of User A and blacklists by assigning FULL_RESTRICTED_STAKER_ROLE role to User A
4. Post cooldown period, User A calls unstake
5. Since unstake does not check for blacklisting status of caller, hence funds get withdrawn to User A chosen recipient, bypassing the blacklisted role

Impact Explanation

FULL_RESTRICTED_STAKER_ROLE can bypass restriction and withdraw funds using unstake

Proof of Concept

Place test at test\unit\concrete\sNUSD\erc4626\sNUSD_erc4626.t.sol

function test_POC() external whenCooldownDurationIsNotZero {        // Setup cooldown		deal(address(sNusd), normalUser, testAmount);        vm.startPrank(normalUser);		nusd.approve(address(sNusd), testAmount);        sNusd.deposit(testAmount, normalUser);        sNusd.cooldownAssets(testAmount);        vm.stopPrank();				vm.startPrank(users.admin);        sNusd.grantRole(FULL_RESTRICTED_STAKER_ROLE, normalUser);        vm.stopPrank();
        // Fast forward time to end of cooldown        uint24 cooldownDuration = sNusd.cooldownDuration();        vm.warp(block.timestamp + cooldownDuration + 1);
        // Now unstake        vm.startPrank(normalUser);        uint256 initialBalance = nusd.balanceOf(normalUser);        sNusd.unstake(normalUser);        uint256 finalBalance = nusd.balanceOf(normalUser);
        // Verify balance increased and cooldown was reset        assertTrue(finalBalance > initialBalance, "NUSD balance should increase after unstaking");
        // Check cooldown was reset        (uint104 cooldownEnd, uint152 underlyingAmount) = sNusd.cooldowns(normalUser);        assertEq(cooldownEnd, 0, "Cooldown end should be reset to zero");        assertEq(underlyingAmount, 0, "Underlying amount should be reset to zero");        vm.stopPrank();    }

Recommendation

Add a check in unstake function to ensure that msg.sender is not having role FULL_RESTRICTED_STAKER_ROLE

Description

Admin can mistakenly add duplicate yield token which is currently accepted. At deletion only the first copy of token in array is deleted, leaving the duplicate one still active. This could allow unintended token to be considered for yield distribution

Proof of Concept

Place the test at yield_distribution.t.sol. Test revert showing token is still active

function test_DuplicateTokenPOC() external whenTheCallerIsDEFAULT_ADMIN_ROLE {	        yieldDistributor.addYieldToken(address(mockUSDC));		yieldDistributor.addYieldToken(address(mockUSDC));
        // Duplicate token addition (start with index 2,3 as setup already use index 0,1)        (address token, bool isActive) = yieldDistributor.yieldTokens(2);		(address token2, bool isActive2) = yieldDistributor.yieldTokens(3);        assertEq(token, address(mockUSDC));		assertEq(token2, address(mockUSDC));
        yieldDistributor.removeYieldToken(address(mockUSDC));
        // Verify token is now inactive		(token2, isActive2) = yieldDistributor.yieldTokens(3);				assertEq(token2, address(mockUSDC));        assertFalse(isActive);    }

Recommendation

Disallow addition of duplicate yield token

Description

It was observed that Request status was not verified while cancelling order. This means even a COMPLETED or CANCELLED order can be re-cancelled

The replay of cancel order causes loss of funds, as everytime order.collateralAmount gets transferred to order.benefactor

Proof of Concept

1. Assume below order at Request id 1

mintRequestStatus[1] = RequestStatus.PENDINGorder.collateralAmount = 1000order.benefactor = User Aorder.collateralAsset = CollA

2. Keeper calls cancelMintRequest to cancel this request id 1
3. This causes 1000 CollA to transferred to User A and status changed to RequestStatus.CANCELLED
4. Lets say, Keeper accidentally again calls cancelMintRequest on request id 1
5. Since there is no status check, cancelMintRequest again causes 1000 CollA to transferred to User A and status changed to RequestStatus.CANCELLED

Note

Similar issue is on cancelRedemptionRequest

Recommendation

Add below check in cancelMintRequest,

if (mintRequestStatus[_requestId] != RequestStatus.PENDING) revert InvalidRequestStatus();

Add below check in cancelRedemptionRequest,

if (redemptionRequestStatus[_requestId] != RequestStatus.PENDING) revert InvalidRequestStatus();

Description

Once the lock expires, the user can withdraw the locked.

However, the code updates the state and external transfer call and subject to reentrancy attack.

IERC20(_asset).safeTransfer(msg.sender, lock.amount);
    delete userLocks[msg.sender][_asset];     totalLocked[_asset] -= lock.amount;

If the locked asset is an ERC777 token with a registered hook, a malicious actor can exploit the reentrancy opportunity by triggering the callback and repeatedly invoking the unlockAsset function, allowing them to withdraw the asset multiple times.

Recommendation

delete userLocks[msg.sender][_asset];totalLocked[_asset] -= lock.amount;
ERC20(_asset).safeTransfer(msg.sender, lock.amount);

Description / Recommendation

Typos

Router.sol

Make below changes for isMintWhitelistEnforced:

- /// @notice Whether the whitelist enforcement is enabled+ /// @notice Whether the mint whitelist enforcement is enabled

Router.sol Make below changes for isMintWhitelisted:

- /// @notice Mapping of user addresses to their whitelist status+ /// @notice Mapping of user addresses to their mint whitelist status

Edges cases & Minor Recommendations

AssetReserve.sol

• Basic sanity check could be added _custodian != address(0) in _setCustodian

sNUSD.sol

• Only allow changing vesting period if unvested amount is 0 else it would impact share price immediately (since totalAssets would change) which would be unexpected by users.

Router.sol

• Since Router is non upgradable, so expiry should be set correctly so that there is no issues later in future integration

Something like:

// where Waiting_Period is configurable by Adminexpiry: block.timestamp + Waiting_Period

Router.sol

• Can include arg startIndex and endIndex in getPendingMintRequests and getPendingRedemptionRequests which would prevent Gas issue if number of request ids becomes too large

sNUSD.sol

• _updateVestingAmount once called is locked for vesting period.
• Thus if redistributeLockedAmount calls _updateVestingAmount then yield distribution via transferInRewards would fail at same time and would only succeed once vesting period is over

function _updateVestingAmount(uint256 _newVestingAmount) internal {        if (getUnvestedAmount() > 0) revert StillVesting();
        vestingAmount = _newVestingAmount;        lastDistributionTimestamp = block.timestamp;    }

Router.sol Router.sol

• Currently, Redemption and Mint above max limit will be queued but would fail on execution causing Keeper to finally cancel the transaction from queue
• Check for _belowMaxRedeemPerBlock and _belowMaxMintPerBlock could also be made at Router level for early revert

sNUSD.sol

• Centralization risk is present at multiple instances in code
• One such instance is by using redistributeLockedAmount. recoverToken does not allow DEFAULT_ADMIN_ROLE to take NUSD but admin still could use redistributeLockedAmount with to as Admin controlled address to get sNUSD.

DeployProtocol.s.sol

Deployment script uses WETH hardcoded address which may change in other chain like Arbitrum. This should be taken care if deployment script need to changed for other chains (along with chain id restriction)

Description

The function aims to mint a tiny share and burn the share to avoid the donation attack.

While router.setMintWhitelisted is necessary to grant deployer mint capacity,

The share is burnt so deployer cannot redeem the share.

Recommendation

Remove router.setRedeemWhitelisted(deployer, true) to save gas.

Description

When mint and redemption requests are canceled, only the _requestId is emitted.

emit MintRequestCancelled(_requestId);

However, when mint and redemption requests are served, the full order detail is still emitted.

emit ServeRequestMint(_requestId, order);

The order details contains a field additionalData and this additionalData can be arbitrarily long.

struct Order {    OrderType orderType;    uint256 expiry;    address benefactor;    address beneficiary;    address collateralAsset;    /// @dev When minting, this is the amount of collateral to deposit    /// @dev When redeeming, this is the minimum amount of collateral to receive    uint256 collateralAmount;    /// @dev When minting, this is the minimum amount of NUSD to receive    /// @dev When redeeming, this is the amount of NUSD to burn    uint256 nusdAmount;    /// @dev Additional data for the order, in case of future use for minters / redeemers     bytes additionalData;

The order keeper must cover the gas costs even when additionalData is excessively long, causing the keeper to consistently incur losses due to high gas consumption.

Recommendation

Only emit requestId when redemption and mint request are served by keeper..

emit ServeRequestMint(_requestId);

and

emit ServeRequestRedemption(_requestId);

Description

When the protocol attempts to mint the initial share to avoid donation attack, both the _collateralAmount, and the _minNusdAmount are 10e18

router.mint(deployer, deploymentArgs.usde, 10e18, 10e18, "");

function mint(        address _beneficiary,        address _collateralAsset,        uint256 _collateralAmount,        uint256 _minNusdAmount,        bytes calldata _additionalData    ) external whenNotPaused enforceMintWhitelist(msg.sender) {

if the USDE traded below 1e18, then the transaction can revert because of 0 slippage tolerance.

Recommendation

uint256 minSlippage = vm.envUint("NEUTRAL_SNUSD__POST_SLIPPAGE_AMOUNT");
IERC20(deploymentArgs.usde).approve(address(router), minSlippage);
router.mint(deployer, deploymentArgs.usde, 10e18, minSlippage, ""); uint256 balance = nusd.balanceOf(deployer);
nusd.approve(deploymentAddress.sNusd, balance);  sNusd.deposit(balance, deployer);
// kill shares with donation to WETH// assertEq(sNusd.totalSupply(), 10e18);  uint256 mintedShare = sNusd.balanceOf(deployer);   sNusd.transfer(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2, mintedShare);   assertEq(sNusd.totalSupply(), mintedShare);
vm.stopBroadcast();