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

Jump to bottom

fix(contracts): OZ-N03 Inconsistent Usage of msg.sender #906

Merged
merged 3 commits into from
Oct 4, 2023
Merged

fix(contracts): OZ-N03 Inconsistent Usage of msg.sender #906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0780e9
use _msgSender instead of msg.sender
zimpha Sep 1, 2023
c28bc1c
merge with develop branch
zimpha Sep 19, 2023
4fd8082
merge with develop branch
zimpha Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions contracts/src/L1/L1ScrollMessenger.sol
View file
Open in desktop
Expand Up @@ -115,7 +115,7 @@ contract L1ScrollMessenger is ScrollMessengerBase, IL1ScrollMessenger {
bytes memory _message,
uint256 _gasLimit
) external payable override whenNotPaused {
_sendMessage(_to, _value, _message, _gasLimit, msg.sender);
_sendMessage(_to, _value, _message, _gasLimit, _msgSender());
}

/// @inheritdoc IScrollMessenger
Expand Down Expand Up @@ -323,7 +323,7 @@ contract L1ScrollMessenger is ScrollMessengerBase, IL1ScrollMessenger {

// compute the actual cross domain message calldata.
uint256 _messageNonce = IL1MessageQueue(_messageQueue).nextCrossDomainMessageIndex();
bytes memory _xDomainCalldata = _encodeXDomainCalldata(msg.sender, _to, _value, _messageNonce, _message);
bytes memory _xDomainCalldata = _encodeXDomainCalldata(_msgSender(), _to, _value, _messageNonce, _message);

// compute and deduct the messaging fee to fee vault.
uint256 _fee = IL1MessageQueue(_messageQueue).estimateCrossDomainMessageFee(_gasLimit);
Expand All @@ -343,7 +343,7 @@ contract L1ScrollMessenger is ScrollMessengerBase, IL1ScrollMessenger {
require(messageSendTimestamp[_xDomainCalldataHash] == 0, "Duplicated message");
messageSendTimestamp[_xDomainCalldataHash] = block.timestamp;

emit SentMessage(msg.sender, _to, _value, _messageNonce, _gasLimit, _message);
emit SentMessage(_msgSender(), _to, _value, _messageNonce, _gasLimit, _message);

// refund fee to `_refundAddress`
unchecked {
Expand Down
24 changes: 14 additions & 10 deletions contracts/src/L1/gateways/L1ERC1155Gateway.sol
View file
Open in desktop
Expand Up @@ -66,7 +66,7 @@ contract L1ERC1155Gateway is ERC1155HolderUpgradeable, ScrollGatewayBase, IL1ERC
uint256 _amount,
uint256 _gasLimit
) external payable override {
_depositERC1155(_token, msg.sender, _tokenId, _amount, _gasLimit);
_depositERC1155(_token, _msgSender(), _tokenId, _amount, _gasLimit);
}

/// @inheritdoc IL1ERC1155Gateway
Expand All @@ -87,7 +87,7 @@ contract L1ERC1155Gateway is ERC1155HolderUpgradeable, ScrollGatewayBase, IL1ERC
uint256[] calldata _amounts,
uint256 _gasLimit
) external payable override {
_batchDepositERC1155(_token, msg.sender, _tokenIds, _amounts, _gasLimit);
_batchDepositERC1155(_token, _msgSender(), _tokenIds, _amounts, _gasLimit);
}

/// @inheritdoc IL1ERC1155Gateway
Expand Down Expand Up @@ -198,19 +198,21 @@ contract L1ERC1155Gateway is ERC1155HolderUpgradeable, ScrollGatewayBase, IL1ERC
address _l2Token = tokenMapping[_token];
require(_l2Token != address(0), "no corresponding l2 token");

address _sender = _msgSender();

// 1. transfer token to this contract
IERC1155Upgradeable(_token).safeTransferFrom(msg.sender, address(this), _tokenId, _amount, "");
IERC1155Upgradeable(_token).safeTransferFrom(_sender, address(this), _tokenId, _amount, "");

// 2. Generate message passed to L2ERC1155Gateway.
bytes memory _message = abi.encodeCall(
IL2ERC1155Gateway.finalizeDepositERC1155,
(_token, _l2Token, msg.sender, _to, _tokenId, _amount)
(_token, _l2Token, _sender, _to, _tokenId, _amount)
);

// 3. Send message to L1ScrollMessenger.
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, msg.sender);
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, _sender);

emit DepositERC1155(_token, _l2Token, msg.sender, _to, _tokenId, _amount);
emit DepositERC1155(_token, _l2Token, _sender, _to, _tokenId, _amount);
}

/// @dev Internal function to batch deposit ERC1155 NFT to layer 2.
Expand All @@ -236,18 +238,20 @@ contract L1ERC1155Gateway is ERC1155HolderUpgradeable, ScrollGatewayBase, IL1ERC
address _l2Token = tokenMapping[_token];
require(_l2Token != address(0), "no corresponding l2 token");

address _sender = _msgSender();

// 1. transfer token to this contract
IERC1155Upgradeable(_token).safeBatchTransferFrom(msg.sender, address(this), _tokenIds, _amounts, "");
IERC1155Upgradeable(_token).safeBatchTransferFrom(_sender, address(this), _tokenIds, _amounts, "");

// 2. Generate message passed to L2ERC1155Gateway.
bytes memory _message = abi.encodeCall(
IL2ERC1155Gateway.finalizeBatchDepositERC1155,
(_token, _l2Token, msg.sender, _to, _tokenIds, _amounts)
(_token, _l2Token, _sender, _to, _tokenIds, _amounts)
);

// 3. Send message to L1ScrollMessenger.
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, msg.sender);
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, _sender);

emit BatchDepositERC1155(_token, _l2Token, msg.sender, _to, _tokenIds, _amounts);
emit BatchDepositERC1155(_token, _l2Token, _sender, _to, _tokenIds, _amounts);
}
}
9 changes: 5 additions & 4 deletions contracts/src/L1/gateways/L1ERC20Gateway.sol
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@ abstract contract L1ERC20Gateway is IL1ERC20Gateway, IMessageDropCallback, Scrol
uint256 _amount,
uint256 _gasLimit
) external payable override {
_deposit(_token, msg.sender, _amount, new bytes(0), _gasLimit);
_deposit(_token, _msgSender(), _amount, new bytes(0), _gasLimit);
}

/// @inheritdoc IL1ERC20Gateway
Expand Down Expand Up @@ -144,11 +144,12 @@ abstract contract L1ERC20Gateway is IL1ERC20Gateway, IMessageDropCallback, Scrol
bytes memory
)
{
address _from = msg.sender;
if (router == msg.sender) {
address _sender = _msgSender();
address _from = _sender;
if (router == _sender) {
// Extract real sender if this call is from L1GatewayRouter.
(_from, _data) = abi.decode(_data, (address, bytes));
_amount = IL1GatewayRouter(msg.sender).requestERC20(_from, _token, _amount);
_amount = IL1GatewayRouter(_sender).requestERC20(_from, _token, _amount);
} else {
// common practice to handle fee on transfer token.
uint256 _before = IERC20Upgradeable(_token).balanceOf(address(this));
Expand Down
24 changes: 14 additions & 10 deletions contracts/src/L1/gateways/L1ERC721Gateway.sol
View file
Open in desktop
Expand Up @@ -64,7 +64,7 @@ contract L1ERC721Gateway is ERC721HolderUpgradeable, ScrollGatewayBase, IL1ERC72
uint256 _tokenId,
uint256 _gasLimit
) external payable override {
_depositERC721(_token, msg.sender, _tokenId, _gasLimit);
_depositERC721(_token, _msgSender(), _tokenId, _gasLimit);
}

/// @inheritdoc IL1ERC721Gateway
Expand All @@ -83,7 +83,7 @@ contract L1ERC721Gateway is ERC721HolderUpgradeable, ScrollGatewayBase, IL1ERC72
uint256[] calldata _tokenIds,
uint256 _gasLimit
) external payable override {
_batchDepositERC721(_token, msg.sender, _tokenIds, _gasLimit);
_batchDepositERC721(_token, _msgSender(), _tokenIds, _gasLimit);
}

/// @inheritdoc IL1ERC721Gateway
Expand Down Expand Up @@ -190,19 +190,21 @@ contract L1ERC721Gateway is ERC721HolderUpgradeable, ScrollGatewayBase, IL1ERC72
address _l2Token = tokenMapping[_token];
require(_l2Token != address(0), "no corresponding l2 token");

address _sender = _msgSender();

// 1. transfer token to this contract
IERC721Upgradeable(_token).safeTransferFrom(msg.sender, address(this), _tokenId);
IERC721Upgradeable(_token).safeTransferFrom(_sender, address(this), _tokenId);

// 2. Generate message passed to L2ERC721Gateway.
bytes memory _message = abi.encodeCall(
IL2ERC721Gateway.finalizeDepositERC721,
(_token, _l2Token, msg.sender, _to, _tokenId)
(_token, _l2Token, _sender, _to, _tokenId)
);

// 3. Send message to L1ScrollMessenger.
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, msg.sender);
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, _sender);

emit DepositERC721(_token, _l2Token, msg.sender, _to, _tokenId);
emit DepositERC721(_token, _l2Token, _sender, _to, _tokenId);
}

/// @dev Internal function to batch deposit ERC721 NFT to layer 2.
Expand All @@ -221,20 +223,22 @@ contract L1ERC721Gateway is ERC721HolderUpgradeable, ScrollGatewayBase, IL1ERC72
address _l2Token = tokenMapping[_token];
require(_l2Token != address(0), "no corresponding l2 token");

address _sender = _msgSender();

// 1. transfer token to this contract
for (uint256 i = 0; i < _tokenIds.length; i++) {
IERC721Upgradeable(_token).safeTransferFrom(msg.sender, address(this), _tokenIds[i]);
IERC721Upgradeable(_token).safeTransferFrom(_sender, address(this), _tokenIds[i]);
}

// 2. Generate message passed to L2ERC721Gateway.
bytes memory _message = abi.encodeCall(
IL2ERC721Gateway.finalizeBatchDepositERC721,
(_token, _l2Token, msg.sender, _to, _tokenIds)
(_token, _l2Token, _sender, _to, _tokenIds)
);

// 3. Send message to L1ScrollMessenger.
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, msg.sender);
IL1ScrollMessenger(messenger).sendMessage{value: msg.value}(counterpart, 0, _message, _gasLimit, _sender);

emit BatchDepositERC721(_token, _l2Token, msg.sender, _to, _tokenIds);
emit BatchDepositERC721(_token, _l2Token, _sender, _to, _tokenIds);
}
}
6 changes: 3 additions & 3 deletions contracts/src/L1/gateways/L1ETHGateway.sol
View file
Open in desktop
Expand Up @@ -44,7 +44,7 @@ contract L1ETHGateway is ScrollGatewayBase, IL1ETHGateway, IMessageDropCallback

/// @inheritdoc IL1ETHGateway
function depositETH(uint256 _amount, uint256 _gasLimit) external payable override {
_deposit(msg.sender, _amount, new bytes(0), _gasLimit);
_deposit(_msgSender(), _amount, new bytes(0), _gasLimit);
}

/// @inheritdoc IL1ETHGateway
Expand Down Expand Up @@ -119,8 +119,8 @@ contract L1ETHGateway is ScrollGatewayBase, IL1ETHGateway, IMessageDropCallback
require(_amount > 0, "deposit zero eth");

// 1. Extract real sender if this call is from L1GatewayRouter.
address _from = msg.sender;
if (router == msg.sender) {
address _from = _msgSender();
if (router == _from) {
(_from, _data) = abi.decode(_data, (address, bytes));
}

Expand Down
17 changes: 9 additions & 8 deletions contracts/src/L1/gateways/L1GatewayRouter.sol
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
}

modifier onlyInContext() {
require(msg.sender == gatewayInContext, "Only in deposit context");
require(_msgSender() == gatewayInContext, "Only in deposit context");
_;
}

Expand Down Expand Up @@ -110,9 +110,10 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
address _token,
uint256 _amount
) external onlyInContext returns (uint256) {
uint256 _balance = IERC20Upgradeable(_token).balanceOf(msg.sender);
IERC20Upgradeable(_token).safeTransferFrom(_sender, msg.sender, _amount);
_amount = IERC20Upgradeable(_token).balanceOf(msg.sender) - _balance;
address _caller = _msgSender();
uint256 _balance = IERC20Upgradeable(_token).balanceOf(_caller);
IERC20Upgradeable(_token).safeTransferFrom(_sender, _caller, _amount);
_amount = IERC20Upgradeable(_token).balanceOf(_caller) - _balance;
return _amount;
}

Expand All @@ -126,7 +127,7 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
uint256 _amount,
uint256 _gasLimit
) external payable override {
depositERC20AndCall(_token, msg.sender, _amount, new bytes(0), _gasLimit);
depositERC20AndCall(_token, _msgSender(), _amount, new bytes(0), _gasLimit);
}

/// @inheritdoc IL1ERC20Gateway
Expand Down Expand Up @@ -154,7 +155,7 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
gatewayInContext = _gateway;

// encode msg.sender with _data
bytes memory _routerData = abi.encode(msg.sender, _data);
bytes memory _routerData = abi.encode(_msgSender(), _data);

IL1ERC20Gateway(_gateway).depositERC20AndCall{value: msg.value}(_token, _to, _amount, _routerData, _gasLimit);

Expand All @@ -180,7 +181,7 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {

/// @inheritdoc IL1ETHGateway
function depositETH(uint256 _amount, uint256 _gasLimit) external payable override {
depositETHAndCall(msg.sender, _amount, new bytes(0), _gasLimit);
depositETHAndCall(_msgSender(), _amount, new bytes(0), _gasLimit);
}

/// @inheritdoc IL1ETHGateway
Expand All @@ -203,7 +204,7 @@ contract L1GatewayRouter is OwnableUpgradeable, IL1GatewayRouter {
require(_gateway != address(0), "eth gateway available");

// encode msg.sender with _data
bytes memory _routerData = abi.encode(msg.sender, _data);
bytes memory _routerData = abi.encode(_msgSender(), _data);

IL1ETHGateway(_gateway).depositETHAndCall{value: msg.value}(_to, _amount, _routerData, _gasLimit);
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/src/L1/gateways/L1WETHGateway.sol
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@ contract L1WETHGateway is L1ERC20Gateway {
}

receive() external payable {
require(msg.sender == WETH, "only WETH");
require(_msgSender() == WETH, "only WETH");
}

/*************************
Expand Down
2 changes: 1 addition & 1 deletion contracts/src/L1/gateways/usdc/L1USDCGateway.sol
View file
Open in desktop
Expand Up @@ -84,7 +84,7 @@ contract L1USDCGateway is L1ERC20Gateway, IUSDCBurnableSourceBridge {

/// @inheritdoc IUSDCBurnableSourceBridge
function burnAllLockedUSDC() external override {
require(msg.sender == circleCaller, "only circle caller");
require(_msgSender() == circleCaller, "only circle caller");

// @note Only bridged USDC will be burned. We may refund the rest if possible.
uint256 _balance = totalBridgedUSDC;
Expand Down
8 changes: 4 additions & 4 deletions contracts/src/L1/rollup/L1MessageQueue.sol
View file
Open in desktop
Expand Up @@ -75,7 +75,7 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
**********************/

modifier onlyMessenger() {
require(msg.sender == messenger, "Only callable by the L1ScrollMessenger");
require(_msgSender() == messenger, "Only callable by the L1ScrollMessenger");
_;
}

Expand Down Expand Up @@ -292,7 +292,7 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
_validateGasLimit(_gasLimit, _data);

// do address alias to avoid replay attack in L2.
address _sender = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
address _sender = AddressAliasHelper.applyL1ToL2Alias(_msgSender());

_queueTransaction(_sender, _target, 0, _gasLimit, _data);
}
Expand All @@ -305,7 +305,7 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
uint256 _gasLimit,
bytes calldata _data
) external override {
require(msg.sender == enforcedTxGateway, "Only callable by the EnforcedTxGateway");
require(_msgSender() == enforcedTxGateway, "Only callable by the EnforcedTxGateway");
// We will check it in EnforcedTxGateway, just in case.
require(_sender.code.length == 0, "only EOA");

Expand All @@ -321,7 +321,7 @@ contract L1MessageQueue is OwnableUpgradeable, IL1MessageQueue {
uint256 _count,
uint256 _skippedBitmap
) external {
require(msg.sender == scrollChain, "Only callable by the ScrollChain");
require(_msgSender() == scrollChain, "Only callable by the ScrollChain");

require(_count <= 256, "pop too many messages");
require(pendingQueueIndex == _startIndex, "start index mismatch");
Expand Down
2 changes: 1 addition & 1 deletion contracts/src/L1/rollup/L2GasPriceOracle.sol
View file
Open in desktop
Expand Up @@ -111,7 +111,7 @@ contract L2GasPriceOracle is OwnableUpgradeable, IL2GasPriceOracle {
/// @notice Allows whitelisted caller to modify the l2 base fee.
/// @param _newL2BaseFee The new l2 base fee.
function setL2BaseFee(uint256 _newL2BaseFee) external {
require(whitelist.isSenderAllowed(msg.sender), "Not whitelisted sender");
require(whitelist.isSenderAllowed(_msgSender()), "Not whitelisted sender");

uint256 _oldL2BaseFee = l2BaseFee;
l2BaseFee = _newL2BaseFee;
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/L1/rollup/ScrollChain.sol
View file
Open in desktop
Expand Up @@ -85,12 +85,12 @@ contract ScrollChain is OwnableUpgradeable, PausableUpgradeable, IScrollChain {

modifier OnlySequencer() {
// @note In the decentralized mode, it should be only called by a list of validator.
require(isSequencer[msg.sender], "caller not sequencer");
require(isSequencer[_msgSender()], "caller not sequencer");
_;
}

modifier OnlyProver() {
require(isProver[msg.sender], "caller not prover");
require(isProver[_msgSender()], "caller not prover");
_;
}

Expand Down
6 changes: 3 additions & 3 deletions contracts/src/L2/L2ScrollMessenger.sol
View file
Open in desktop
Expand Up @@ -92,7 +92,7 @@ contract L2ScrollMessenger is ScrollMessengerBase, IL2ScrollMessenger {
bytes memory _message
) external override whenNotPaused {
// It is impossible to deploy a contract with the same address, reentrance is prevented in nature.
require(AddressAliasHelper.undoL1ToL2Alias(msg.sender) == counterpart, "Caller is not L1ScrollMessenger");
require(AddressAliasHelper.undoL1ToL2Alias(_msgSender()) == counterpart, "Caller is not L1ScrollMessenger");

bytes32 _xDomainCalldataHash = keccak256(_encodeXDomainCalldata(_from, _to, _value, _nonce, _message));

Expand Down Expand Up @@ -120,15 +120,15 @@ contract L2ScrollMessenger is ScrollMessengerBase, IL2ScrollMessenger {
_addUsedAmount(_value);

uint256 _nonce = L2MessageQueue(messageQueue).nextMessageIndex();
bytes32 _xDomainCalldataHash = keccak256(_encodeXDomainCalldata(msg.sender, _to, _value, _nonce, _message));
bytes32 _xDomainCalldataHash = keccak256(_encodeXDomainCalldata(_msgSender(), _to, _value, _nonce, _message));

// normally this won't happen, since each message has different nonce, but just in case.
require(messageSendTimestamp[_xDomainCalldataHash] == 0, "Duplicated message");
messageSendTimestamp[_xDomainCalldataHash] = block.timestamp;

L2MessageQueue(messageQueue).appendMessage(_xDomainCalldataHash);

emit SentMessage(msg.sender, _to, _value, _nonce, _gasLimit, _message);
emit SentMessage(_msgSender(), _to, _value, _nonce, _gasLimit, _message);
}

/// @dev Internal function to execute a L1 => L2 message.
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/L2/gateways/L2CustomERC20Gateway.sol
View file
Open in desktop
Expand Up @@ -121,8 +121,8 @@ contract L2CustomERC20Gateway is L2ERC20Gateway {
require(_amount > 0, "withdraw zero amount");

// 1. Extract real sender if this call is from L2GatewayRouter.
address _from = msg.sender;
if (router == msg.sender) {
address _from = _msgSender();
if (router == _from) {
(_from, _data) = abi.decode(_data, (address, bytes));
}

Expand Down