Ethena Labs - ThreeSigma's results

Low

[L-1] Functions permissions aren't the same as documented

In the contract StakedUSDe.sol the functions addToBlacklist() and removeFromBlacklist() both say in the NatSpec comments that the owner and the blacklist manager can call these functions. However the code only allows the blacklist manager to call these functions.

Non-Critical Issues

[N-1] Misleading modifier names

In SingleAdminAccessControl.sol the name of the modifier notAdmin() should be more specific (i.e. roleNotAdmin()). The current name is too generic, leading to confusion and could be interpreted as what is being check is if the msg.sender is the admin (as often seen in other protocols).

The same issue appears in the StakedUSDe.sol contract, where a modifier with a similar name is used here (notOwner()), but now what is being checked is if the blacklist target is not the owner. Thus, a more clear modifier name could be blacklistTargetNotOwner().

[N-2] Misleading error message

In SingelAdminAccessControl.sol, in line 18, the error message does not match what is happening in the modifier. This modifier checks if a given role is the DEFAULT_ADMIN_ROLE, and thus the error InvalidAdminChange should be replaced with a more suitable error such as RoleIsAdmin.

[N-3] Use of wrong interface

In USDeSilo.sol when establishing the USDe contract here, the IERC20 interface is used. This is fine since for now the only function called in the contract is the transfer function that is in the IERC20 interface. However, since there is a IUSDe interface already written that extends IERC20 it should be used here for clarity purposes and for future uses.

[GAS-1] Add internal functions to remove extra logic.

In the constructor of EthenaMinting.sol the role of DEFAULT_ADMIN_ROLE is first given to msg.sender and then to _admin. This is done in the constructor so the functions addSupportedAsset() and addCustodianAddress() can be called since they have the condition onlyRole(DEFAULT_ADMIN_ROLE). The contract can be optimized by creating internal functions for adding the assets and custodian addresses that do not have the modifier, which will be called in the constructor and in the public add functions which wil keep the modifier. This way in the constructor you only need to call _grantRole() once for the _admin without the if condition saving on gas.

[GAS-2] Remove return value since it will always be the same.

In the contract EthenaMinting.sol the functions VerifyOrder() and verifyNonce() return a boolean along other variables. This boolean will always be true so it is not needed. If the call doesn't revert we know that the verification worked so the boolean is unnecessary.

[GAS-3] Unnecessary parameter and verification.

In the contract EthenaMinting.sol the function VerifyRoute() receives the orderType and proceeds to check if it is a REDEEM if so it returns true. This function is only called internally from the mint() function, and if someone calls the function publicly they wouldn't say it is for a REDEEM as the route is only needed for the mint. Therefore there is no need to receive the orderType in the function nor is its verification.

[GAS-4] Unnecessary creation of storage variable in VerifyNonce()

In the contract EthenaMinting.sol the function VerifyNonce() creates the variable invalidatorStorage unnecessarily. It would be less gas to simply replace lines 381 and 382 with uint256 invalidator = _orderBitmaps[sender][invalidatorSlot];

[GAS-5] Unnecessary creation of storage variable in _deduplicateOrder()

In the contract EthenaMinting.sol the function _deduplicateOrder() creates the variable invalidatorStorage unnecessarily. It would be less gas to simply replace the lines 393 and 394 with _orderBitmaps[sender][invalidatorSlot] = invalidator | invalidatorBit;.

[GAS-6] Adding a variable that will always be zero.

In the contract StakedUSDe.sol the function transferInRewards() checks that getUnvestedAmount() returns 0. In the next line it calls getUnvestedAmount() again to add it to a variable, but we already know it will return 0. Remove the second call to getUnvestedAmount().