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.

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

fix: use custom errors to reduce gas consumption #187

Merged
merged 13 commits into from
May 8, 2023
Merged
44 changes: 32 additions & 12 deletions contracts/Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,33 @@ contract Bridge is Pausable, Context, EIP712 {

event Retry(string txHash);

error AccessNotAllowed(address sender, bytes4 funcSig);

error ResourceIDNotMappedToHandler();

error DepositToCurrentDomain();

error InvalidProposalSigner();

error EmptyProposalsArray();

error NonceDecrementsNotAllowed();

error MPCAddressAlreadySet();

error MPCAddressNotSet();

error MPCAddressIsNotUpdatable();

error MPCAddressZeroAddress();

modifier onlyAllowed() {
_onlyAllowed(msg.sig, _msgSender());
_;
}

function _onlyAllowed(bytes4 sig, address sender) private view {
require(_accessControl.hasAccess(sig, sender), "sender doesn't have access to function");
if (!_accessControl.hasAccess(sig, sender)) revert AccessNotAllowed(sender, sig);
}

function _msgSender() internal override view returns (address) {
Expand Down Expand Up @@ -127,7 +147,7 @@ contract Bridge is Pausable, Context, EIP712 {
@notice MPC address has to be set before Bridge can be unpaused
*/
function adminUnpauseTransfers() external onlyAllowed {
require(_MPCAddress != address(0), "MPC address not set");
if (_MPCAddress == address(0)) revert MPCAddressNotSet();
_unpause(_msgSender());
}

Expand Down Expand Up @@ -228,16 +248,16 @@ contract Bridge is Pausable, Context, EIP712 {
@param feeData Additional data to be passed to the fee handler.
@notice Emits {Deposit} event with all necessary parameters and a handler response.
@return depositNonce deposit nonce for the destination domain.
@return handlerResponse a handler response:
@return handlerResponse a handler response:
- ERC20Handler: responds with an empty data.
- ERC721Handler: responds with the deposited token metadata acquired by calling a tokenURI method in the token contract.
- PermissionedGenericHandler: responds with the raw bytes returned from the call to the target contract.
- PermissionlessGenericHandler: responds with an empty data.
*/
function deposit(uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData)
external payable whenNotPaused
function deposit(uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData)
external payable whenNotPaused
returns (uint64 depositNonce, bytes memory handlerResponse) {
require(destinationDomainID != _domainID, "Can't deposit to current domain");
if (destinationDomainID == _domainID) revert DepositToCurrentDomain();

address sender = _msgSender();
if (address(_feeHandler) == address(0)) {
Expand All @@ -247,7 +267,7 @@ contract Bridge is Pausable, Context, EIP712 {
_feeHandler.collectFee{value: msg.value}(sender, _domainID, destinationDomainID, resourceID, depositData, feeData);
}
address handler = _resourceIDToHandlerAddress[resourceID];
require(handler != address(0), "resourceID not mapped to handler");
if (handler == address(0)) revert ResourceIDNotMappedToHandler();

depositNonce = ++_depositCounts[destinationDomainID];

Expand Down Expand Up @@ -294,8 +314,8 @@ contract Bridge is Pausable, Context, EIP712 {
In the case of {PermissionedGenericHandler}, when execution fails, the handler will emit a failure event and terminate the function normally.
*/
function executeProposals(Proposal[] memory proposals, bytes calldata signature) public whenNotPaused {
require(proposals.length > 0, "Proposals can't be an empty array");
require(verify(proposals, signature), "Invalid proposal signer");
if (proposals.length == 0) revert EmptyProposalsArray();
if (!verify(proposals, signature)) revert InvalidProposalSigner();

for (uint256 i = 0; i < proposals.length; i++) {
if(isProposalExecuted(proposals[i].originDomainID, proposals[i].depositNonce)) {
Expand Down Expand Up @@ -327,7 +347,7 @@ contract Bridge is Pausable, Context, EIP712 {
which is mapped in {functionAccess} in AccessControlSegregator contract.
*/
function startKeygen() external onlyAllowed {
require(_MPCAddress == address(0), "MPC address is already set");
if (_MPCAddress != address(0)) revert MPCAddressAlreadySet();
emit StartKeygen();
}

Expand All @@ -339,8 +359,8 @@ contract Bridge is Pausable, Context, EIP712 {
@param MPCAddress Address that will be set as MPC address.
*/
function endKeygen(address MPCAddress) external onlyAllowed {
require(MPCAddress != address(0), "MPC address can't be null-address");
require(_MPCAddress == address(0), "MPC address can't be updated");
if( MPCAddress == address(0)) revert MPCAddressZeroAddress();
if (_MPCAddress != address(0)) revert MPCAddressIsNotUpdatable();
_MPCAddress = MPCAddress;
_unpause(_msgSender());
emit EndKeygen();
Expand Down
1 change: 0 additions & 1 deletion contracts/ERC20Safe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,4 @@ contract ERC20Safe {
require(abi.decode(returndata, (bool)), "ERC20: operation did not succeed");
nmlinaric marked this conversation as resolved.
Show resolved Hide resolved
}
}

}
32 changes: 31 additions & 1 deletion contracts/TestContracts.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.11;

import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Wrapper.sol";
import "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetMinterPauser.sol";
import "./handlers/ERCHandlerHelpers.sol";
import "./interfaces/IERC20Plus.sol";
Expand Down Expand Up @@ -160,6 +159,37 @@ contract XC20Test is ERC20 {
}
}

/**
@dev This contract mocks XC20Test where "transferFrom()" always fails
*/
contract XC20TestMock is XC20Test {

function transferFrom(address from, address to, uint256 amount) public virtual override(ERC20) returns (bool) {
address spender = _msgSender();
_spendAllowance(from, spender, amount);
_transfer(from, to, amount);
return false;
}
}

/**
@dev This contract mocks ERC20PresetMinterPauser where and "transferFrom()" always fails
*/
contract ERC20PresetMinterPauserMock is ERC20PresetMinterPauser {

constructor(
string memory name,
string memory symbol
) ERC20PresetMinterPauser(name, symbol) {}

function transferFrom(address from, address to, uint256 amount) public virtual override(ERC20) returns (bool) {
address spender = _msgSender();
_spendAllowance(from, spender, amount);
_transfer(from, to, amount);
return false;
}
}

contract ERC20PresetMinterPauserDecimals is ERC20PresetMinterPauser {

uint8 private immutable customDecimals;
Expand Down
2 changes: 1 addition & 1 deletion contracts/handlers/ERC1155Handler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ contract ERC1155Handler is IHandler, ERCHandlerHelpers, ERC1155Safe, ERC1155Hold
}

address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
require(_tokenContractAddressToTokenProperties[address(tokenAddress)].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
mintBatchERC1155(tokenAddress, address(recipientAddress), tokenIDs, amounts, transferData);
Expand Down
4 changes: 2 additions & 2 deletions contracts/handlers/ERC20Handler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, ERC20Safe {
(amount) = abi.decode(data, (uint));

address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
burnERC20(tokenAddress, depositor, amount);
Expand Down Expand Up @@ -79,7 +79,7 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, ERC20Safe {
recipientAddress := mload(add(destinationRecipientAddress, 0x20))
}

require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
mintERC20(tokenAddress, address(recipientAddress), convertToExternalBalance(tokenAddress, amount));
Expand Down
4 changes: 2 additions & 2 deletions contracts/handlers/ERC721Handler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
(tokenID) = abi.decode(data, (uint));

address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

// Check if the contract supports metadata, fetch it if it does
if (tokenAddress.supportsInterface(_INTERFACE_ERC721_METADATA)) {
Expand Down Expand Up @@ -103,7 +103,7 @@ contract ERC721Handler is IHandler, ERCHandlerHelpers, ERC721Safe {
}

address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
require(_tokenContractAddressToTokenProperties[address(tokenAddress)].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
mintERC721(tokenAddress, address(recipientAddress), tokenID, metaData);
Expand Down
6 changes: 4 additions & 2 deletions contracts/handlers/ERCHandlerHelpers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ contract ERCHandlerHelpers is IERCHandler {
Decimals decimals;
}

error ContractAddressNotWhitelisted(address contractAddress);

// resourceID => token contract address
mapping (bytes32 => address) public _resourceIDToTokenContractAddress;

Expand Down Expand Up @@ -71,7 +73,7 @@ contract ERCHandlerHelpers is IERCHandler {
}

function _setBurnable(address contractAddress) internal {
require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contract is not whitelisted");
if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);
_tokenContractAddressToTokenProperties[contractAddress].isBurnable = true;
}

Expand All @@ -83,7 +85,7 @@ contract ERCHandlerHelpers is IERCHandler {
@param externalDecimals Decimal places of token that is transferred.
*/
function _setDecimals(address contractAddress, uint8 externalDecimals) internal {
require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contract is not whitelisted");
if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);
_tokenContractAddressToTokenProperties[contractAddress].decimals = Decimals({
isSet: true,
externalDecimals: externalDecimals
Expand Down
6 changes: 4 additions & 2 deletions contracts/handlers/PermissionedGenericHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ contract PermissionedGenericHandler is IHandler {
bool isWhitelisted;
}

error ContractAddressNotWhitelisted(address contractAddress);

// token contract address => TokenContractProperties
mapping (address => TokenContractProperties) public _tokenContractAddressToTokenProperties;

Expand Down Expand Up @@ -106,7 +108,7 @@ contract PermissionedGenericHandler is IHandler {
require(depositor == address(uint160(metadataDepositor >> 96)), 'incorrect depositor in the data');
}

require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contractAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);

bytes4 sig = _tokenContractAddressToTokenProperties[contractAddress].depositFunctionSignature;
if (sig != bytes4(0)) {
Expand Down Expand Up @@ -136,7 +138,7 @@ contract PermissionedGenericHandler is IHandler {
metaData = bytes(data[32:32 + lenMetadata]);

address contractAddress = _resourceIDToContractAddress[resourceID];
require(_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted, "provided contractAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[contractAddress].isWhitelisted) revert ContractAddressNotWhitelisted(contractAddress);

bytes4 sig = _tokenContractAddressToTokenProperties[contractAddress].executeFunctionSignature;
if (sig != bytes4(0)) {
Expand Down
4 changes: 2 additions & 2 deletions contracts/handlers/XC20Handler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
(amount) = abi.decode(data, (uint));

address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
burnERC20(tokenAddress, depositor, amount);
Expand Down Expand Up @@ -79,7 +79,7 @@ contract XC20Handler is IHandler, ERCHandlerHelpers, XC20Safe {
recipientAddress := mload(add(destinationRecipientAddress, 0x20))
}

require(_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted, "provided tokenAddress is not whitelisted");
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted) revert ContractAddressNotWhitelisted(tokenAddress);

if (_tokenContractAddressToTokenProperties[tokenAddress].isBurnable) {
mintERC20(tokenAddress, address(recipientAddress), convertToExternalBalance(tokenAddress, amount));
Expand Down
4 changes: 3 additions & 1 deletion contracts/handlers/fee/BasicFeeHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ contract BasicFeeHandler is IFeeHandler, AccessControl {
uint256 newFee
);

error IncorrectFeeSupplied(uint256);

modifier onlyAdmin() {
require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "sender doesn't have admin role");
_;
Expand Down Expand Up @@ -69,7 +71,7 @@ contract BasicFeeHandler is IFeeHandler, AccessControl {
@param feeData Additional data to be passed to the fee handler.
*/
function collectFee(address sender, uint8 fromDomainID, uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData) payable external onlyBridgeOrRouter {
require(msg.value == _fee, "Incorrect fee supplied");
if (msg.value != _fee) revert IncorrectFeeSupplied(msg.value);
emit FeeCollected(sender, fromDomainID, destinationDomainID, resourceID, _fee, address(0));
}

Expand Down
20 changes: 13 additions & 7 deletions contracts/handlers/fee/DynamicERC20FeeHandlerEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ contract DynamicERC20FeeHandlerEVM is DynamicFeeHandler {
total: 353
*/

require(feeData.length == 353, "Incorrect feeData length");
if (feeData.length != 353) revert IncorrectFeeDataLength(feeData.length);

FeeDataType memory feeDataDecoded;
uint256 txCost;
Expand All @@ -66,12 +66,18 @@ contract DynamicERC20FeeHandlerEVM is DynamicFeeHandler {
feeDataDecoded.sig = bytes(feeData[256: 321]);

OracleMessageType memory oracleMessage = abi.decode(feeDataDecoded.message, (OracleMessageType));
require(block.timestamp <= oracleMessage.expiresAt, "Obsolete oracle data");
require((oracleMessage.fromDomainID == fromDomainID)
&& (oracleMessage.toDomainID == destinationDomainID)
&& (oracleMessage.resourceID == resourceID),
"Incorrect deposit params"
);
if (block.timestamp > oracleMessage.expiresAt) revert ObsoleteOracleData();
if ((oracleMessage.fromDomainID != fromDomainID) ||
(oracleMessage.toDomainID != destinationDomainID) ||
(oracleMessage.resourceID != resourceID)
) {
revert IncorrectDepositParams(
oracleMessage.fromDomainID,
oracleMessage.toDomainID,
oracleMessage.resourceID
);
}


bytes32 messageHash = keccak256(feeDataDecoded.message);

Expand Down
19 changes: 12 additions & 7 deletions contracts/handlers/fee/DynamicERC20FeeHandlerSubstrate.sol
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ contract DynamicERC20FeeHandlerSubstrate is DynamicFeeHandler {
total: 353
*/

require(feeData.length == 353, "Incorrect feeData length");
if (feeData.length != 353) revert IncorrectFeeDataLength(feeData.length);

FeeDataType memory feeDataDecoded;
uint256 txCost;
Expand All @@ -80,12 +80,17 @@ contract DynamicERC20FeeHandlerSubstrate is DynamicFeeHandler {
feeDataDecoded.sig = bytes(feeData[256: 321]);

SubstrateOracleMessageType memory oracleMessage = abi.decode(feeDataDecoded.message, (SubstrateOracleMessageType));
require(block.timestamp <= oracleMessage.expiresAt, "Obsolete oracle data");
require((oracleMessage.fromDomainID == fromDomainID)
&& (oracleMessage.toDomainID == destinationDomainID)
&& (oracleMessage.resourceID == resourceID),
"Incorrect deposit params"
);
if (block.timestamp > oracleMessage.expiresAt) revert ObsoleteOracleData();
if ((oracleMessage.fromDomainID != fromDomainID) ||
(oracleMessage.toDomainID != destinationDomainID) ||
(oracleMessage.resourceID != resourceID)
) {
revert IncorrectDepositParams(
oracleMessage.fromDomainID,
oracleMessage.toDomainID,
oracleMessage.resourceID
);
}

bytes32 messageHash = keccak256(feeDataDecoded.message);

Expand Down
14 changes: 12 additions & 2 deletions contracts/handlers/fee/DynamicFeeHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ abstract contract DynamicFeeHandler is IFeeHandler, AccessControl, ERC20Safe {

event FeeOraclePropertiesSet(uint32 gasUsed, uint16 feePercent);

error InvalidSignature();

error IncorrectFeeDataLength(uint256);

error IncorrectFeeSupplied(uint256);

error ObsoleteOracleData();

error IncorrectDepositParams(uint8 fromDomainID, uint8 destinationDomainID, bytes32 resourceID);

modifier onlyAdmin() {
require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "sender doesn't have admin role");
_;
Expand Down Expand Up @@ -121,7 +131,7 @@ abstract contract DynamicFeeHandler is IFeeHandler, AccessControl, ERC20Safe {
function collectFee(address sender, uint8 fromDomainID, uint8 destinationDomainID, bytes32 resourceID, bytes calldata depositData, bytes calldata feeData) payable external onlyBridgeOrRouter {
(uint256 fee, address tokenAddress) = _calculateFee(sender, fromDomainID, destinationDomainID, resourceID, depositData, feeData);
if(tokenAddress == address(0)){
require(msg.value == fee, "Incorrect fee supplied");
if (msg.value != fee) revert IncorrectFeeSupplied(msg.value);
} else {
require(msg.value == 0, "collectFee: msg.value != 0");
lockERC20(tokenAddress, sender, address(this), fee);
Expand Down Expand Up @@ -166,6 +176,6 @@ abstract contract DynamicFeeHandler is IFeeHandler, AccessControl, ERC20Safe {

function verifySig(bytes32 message, bytes memory signature, address signerAddress) internal pure {
address signerAddressRecovered = ECDSA.recover(message, signature);
require(signerAddressRecovered == signerAddress, 'Invalid signature');
if (signerAddressRecovered != signerAddress) revert InvalidSignature();
}
}
Loading