Contracts minor audit by 0xHattoriHanzo #24
Conversation
|
@RuslanProgrammer @FedOken For your review. |
|
Thanks. I've tagged some of the open source Smart Contract devs to review. |
|
We agree with your comments. We are glad you suggested these changes. The smart contracts are already being deployed. We will not merge this yet, but we will consider these suggestions in a possible new version. |
| @@ -172,10 +175,10 @@ contract Distribution is IDistribution, OwnableUpgradeable, UUPSUpgradeable { | |||
| userData.rate = currentPoolRate_; | |||
| userData.pendingRewards = 0; | |||
|
|
|||
| emit UserClaimed(poolId_, user_, receiver_, pendingRewards_); | |||
There was a problem hiding this comment.
What is the purpose of replacing the emit location?
| @@ -257,7 +260,7 @@ contract Distribution is IDistribution, OwnableUpgradeable, UUPSUpgradeable { | |||
| newDeposited_ = deposited_ - amount_; | |||
|
|
|||
| require(amount_ > 0, "DS: nothing to withdraw"); | |||
| require(newDeposited_ >= pool.minimalStake || newDeposited_ == 0, "DS: invalid withdraw amount"); | |||
| require(newDeposited_ >= pool.minimalStake || newDeposited_ <= 0, "DS: invalid withdraw amount"); | |||
There was a problem hiding this comment.
newDeposited_ can't be less than 0. It will revert higher in this case.
| @@ -56,27 +57,27 @@ contract L1Sender is IL1Sender, OwnableUpgradeable, UUPSUpgradeable { | |||
| function setDepositTokenConfig(DepositTokenConfig calldata newConfig_) public onlyOwner { | |||
| require(newConfig_.receiver != address(0), "L1S: invalid receiver"); | |||
|
|
|||
| DepositTokenConfig storage oldConfig = depositTokenConfig; | |||
| DepositTokenConfig memory oldConfig = depositTokenConfig; | |||
There was a problem hiding this comment.
Why you replace storage -> memory? We don't use oldConfig.receiver;
| DepositTokenConfig storage oldConfig = depositTokenConfig; | ||
| DepositTokenConfig memory oldConfig = depositTokenConfig; | ||
|
|
||
| depositTokenConfig = newConfig_; |
There was a problem hiding this comment.
Why you change storage here?
| } | ||
|
|
||
| function _replaceDepositToken(address oldToken_, address newToken_) private { | ||
| bool isTokenChanged_ = oldToken_ != newToken_; | ||
|
|
||
| if (oldToken_ != address(0) && isTokenChanged_) { | ||
| // Remove allowance from stETH to wstETH | ||
| IERC20(unwrappedDepositToken).approve(oldToken_, 0); | ||
| require(IERC20(unwrappedDepositToken).approve(oldToken_, 0), "L1S: remove old token allowance failed"); |
There was a problem hiding this comment.
Could you add a test that throws this exception? I'd love to see it.
approve() does return a bool, but in case of an error, there is no way to return the value from approve(). That's why I don't see any sense in making a check in this place.
| delete failedMessages[senderChainId_][senderAndReceiverAddresses_][nonce_]; | ||
|
|
||
| emit RetryMessageSuccess(senderChainId_, senderAndReceiverAddresses_, nonce_, payload_); | ||
|
|
There was a problem hiding this comment.
The same here, why we mint tokens before clearing failed message storage?
|
Should these changes included now? Or wait for a future re-deployment? |
Hello Morpheus,
In order to get started and familiar with the project, I have performed a minor security audit in the smart contracts.
Luckily I have not been able to find any big issue despite some minor enhancements that I will describe below:
Missing
IERC20.approvereturn values. As a good practice, the ERC20 approve function returns a bool indicating if the function call has been performed successfully.Added missing 0x address validations in some functions
Reduced reentrancy attack exposure for state changes and events