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.

Sign up for GitHub

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

Contracts minor audit by 0xHattoriHanzo #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Contracts minor audit by 0xHattoriHanzo #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions contracts/Distribution.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ contract Distribution is IDistribution, OwnableUpgradeable, UUPSUpgradeable {
createPool(poolsInfo_[i]);
}

require(depositToken_ != address(0), "DS: invalid depositToken_ value");
require(l1Sender_ != address(0), "DS: invalid l1Sender_ value");

depositToken = depositToken_;
l1Sender = l1Sender_;
}
Expand Down Expand Up @@ -172,10 +175,10 @@ contract Distribution is IDistribution, OwnableUpgradeable, UUPSUpgradeable {
userData.rate = currentPoolRate_;
userData.pendingRewards = 0;

emit UserClaimed(poolId_, user_, receiver_, pendingRewards_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of replacing the emit location?


// Transfer rewards
L1Sender(l1Sender).sendMintMessage{value: msg.value}(receiver_, pendingRewards_, user_);

emit UserClaimed(poolId_, user_, receiver_, pendingRewards_);
}

function withdraw(uint256 poolId_, uint256 amount_) external poolExists(poolId_) poolPublic(poolId_) {
Expand Down Expand Up @@ -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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

newDeposited_ can't be less than 0. It will revert higher in this case.

} else {
newDeposited_ = deposited_ - amount_;
}
Expand Down
21 changes: 14 additions & 7 deletions contracts/L1Sender.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ contract L1Sender is IL1Sender, OwnableUpgradeable, UUPSUpgradeable {
}

function setDistribution(address distribution_) public onlyOwner {
require(distribution_ != address(0), "L1S: invalid distribution");
distribution = distribution_;
}

Expand All @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you replace storage -> memory? We don't use oldConfig.receiver;


depositTokenConfig = newConfig_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you change storage here?


_replaceDepositToken(oldConfig.token, newConfig_.token);
_replaceDepositTokenGateway(oldConfig.gateway, newConfig_.gateway, oldConfig.token, newConfig_.token);

depositTokenConfig = newConfig_;
}

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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

if (isTokenChanged_) {
// Get stETH from wstETH
address unwrappedToken_ = IWStETH(newToken_).stETH();
// Increase allowance from stETH to wstETH. To exchange stETH for wstETH
IERC20(unwrappedToken_).approve(newToken_, type(uint256).max);
require(IERC20(unwrappedToken_).approve(newToken_, type(uint256).max), "L1S: new token allowance failed");

unwrappedDepositToken = unwrappedToken_;
}
Expand All @@ -91,11 +92,17 @@ contract L1Sender is IL1Sender, OwnableUpgradeable, UUPSUpgradeable {
bool isAllowedChanged_ = (oldToken_ != newToken_) || (oldGateway_ != newGateway_);

if (oldGateway_ != address(0) && isAllowedChanged_) {
IERC20(oldToken_).approve(IGatewayRouter(oldGateway_).getGateway(oldToken_), 0);
require(
IERC20(oldToken_).approve(IGatewayRouter(oldGateway_).getGateway(oldToken_), 0),
"L1S: remove old gateway allowance failed"
);
}

if (isAllowedChanged_) {
IERC20(newToken_).approve(IGatewayRouter(newGateway_).getGateway(newToken_), type(uint256).max);
require(
IERC20(newToken_).approve(IGatewayRouter(newGateway_).getGateway(newToken_), type(uint256).max),
"L1S: new gateway allowance failed"
);
}
}

Expand Down
5 changes: 3 additions & 2 deletions contracts/L2MessageReceiver.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ contract L2MessageReceiver is IL2MessageReceiver, OwnableUpgradeable, UUPSUpgrad
}

function setParams(address rewardToken_, Config calldata config_) external onlyOwner {
require(rewardToken_ != address(0), "L2MR: invalid reward token");
rewardToken = rewardToken_;
config = config_;
}
Expand Down Expand Up @@ -59,11 +60,11 @@ contract L2MessageReceiver is IL2MessageReceiver, OwnableUpgradeable, UUPSUpgrad
require(payloadHash_ != bytes32(0), "L2MR: no stored message");
require(keccak256(payload_) == payloadHash_, "L2MR: invalid payload");

_nonblockingLzReceive(senderChainId_, senderAndReceiverAddresses_, payload_);

delete failedMessages[senderChainId_][senderAndReceiverAddresses_][nonce_];

emit RetryMessageSuccess(senderChainId_, senderAndReceiverAddresses_, nonce_, payload_);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same here, why we mint tokens before clearing failed message storage?

_nonblockingLzReceive(senderChainId_, senderAndReceiverAddresses_, payload_);
}

function _blockingLzReceive(
Expand Down
7 changes: 5 additions & 2 deletions contracts/L2TokenReceiver.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ contract L2TokenReceiver is IL2TokenReceiver, OwnableUpgradeable, UUPSUpgradeabl
__Ownable_init();
__UUPSUpgradeable_init();

require(router_ != address(0), "L2TR: invalid router_ value");
require(nonfungiblePositionManager_ != address(0), "L2TR: invalid nonfungiblePositionManager_ value");

router = router_;
nonfungiblePositionManager = nonfungiblePositionManager_;

Expand Down Expand Up @@ -144,12 +147,12 @@ contract L2TokenReceiver is IL2TokenReceiver, OwnableUpgradeable, UUPSUpgradeabl
require(newParams_.tokenIn != address(0), "L2TR: invalid tokenIn");
require(newParams_.tokenOut != address(0), "L2TR: invalid tokenOut");

params = newParams_;

TransferHelper.safeApprove(newParams_.tokenIn, router, type(uint256).max);
TransferHelper.safeApprove(newParams_.tokenIn, nonfungiblePositionManager, type(uint256).max);

TransferHelper.safeApprove(newParams_.tokenOut, nonfungiblePositionManager, type(uint256).max);

params = newParams_;
}

function _authorizeUpgrade(address) internal view override onlyOwner {}
Expand Down