LBTC - Lombard

The contracts in-scope are safe-guarded by an access control mechanism, which is a good practice in smart contract development and prevents non-authorized parties and bad actors to perform unauthorized activities within the contracts and ecosystem.

Owners with privileged rights to perform administrative operations need to be trusted and have power mitigated to prevent unilateral actions.

- contracts/LBTC/LBTC.sol

function toggleWithdrawals() external onlyOwner {

- contracts/LBTC/LBTC.sol

function changeNameAndSymbol(string calldata name_, string calldata symbol_) external onlyOwner {

- contracts/LBTC/LBTC.sol

function changeConsortium(address newVal) external onlyOwner {

- contracts/LBTC/LBTC.sol

function addDestination(bytes32 toChain, bytes32 toContract, uint16 relCommission, uint64 absCommission) external onlyOwner {

- contracts/LBTC/LBTC.sol

function removeDestination(bytes32 toChain) external onlyOwner {

- contracts/LBTC/LBTC.sol

function changeBascule(address newVal) external onlyOwner {

- contracts/LBTC/LBTC.sol

function transferPauserRole(address newPauser) external onlyOwner {

- contracts/bascule/Bascule.sol

function pause() public onlyRole(PAUSER_ROLE) {

- contracts/bascule/Bascule.sol

function unpause() public onlyRole(PAUSER_ROLE) {

- contracts/bascule/Bascule.sol

function setMaxDeposits(uint256 aMaxDeposits) public whenNotPaused onlyRole(DEFAULT_ADMIN_ROLE) {

- contracts/bascule/Bascule.sol

) public whenNotPaused onlyRole(DEPOSIT_REPORTER_ROLE) {

- contracts/bascule/Bascule.sol

) public whenNotPaused onlyRole(WITHDRAWAL_VALIDATOR_ROLE) {

- contracts/consortium/LombardConsortium.sol

function changeThresholdAddr(address newVal) external onlyOwner {

It is recommended to enforce a Multi-signature mechanism, in order to mitigate potential centralization concerns regarding unilateral administrative tasks.

Remediation Plan

SOLVED: The Lombard team has solved this issue, by enforcing the usage of Safe multi-signature wallets for administrative tasks.

The current implementation of the EIP1271SignatureUtils.checkSignature function contains divergences from the standard ERC1271 specification. Specifically, the address signer is always set to the address of the configured Consortium. Consequently, the if (isContract(signer)) statement will always return true, as it is expected, that deployed code exists at the specified address. As a result, the condition specified in the else block will never execute.

- evm-smart-contracts/contracts/LBTC/LBTC.sol

    function _checkAndUseProof(LBTCStorage storage self, bytes calldata data, bytes calldata proofSignature) internal returns (bytes32 proofHash) {
        proofHash = keccak256(data);

        // we can trust data only if proof is signed by Consortium
        EIP1271SignatureUtils.checkSignature(self.consortium, proofHash, proofSignature);
        // We can save the proof, because output with index in unique pair
        if (self.usedProofs[proofHash]) {
            revert ProofAlreadyUsed();
        }
        self.usedProofs[proofHash] = true;
    }

- evm-smart-contracts/contracts/libs/EIP1271SignatureUtils.sol

    function checkSignature(address signer, bytes32 digestHash, bytes memory signature) internal view {

        if (isContract(signer)) {
            if (IERC1271(signer).isValidSignature(digestHash, signature) != EIP1271_MAGICVALUE) {
                revert SignatureVerificationFailed();
            }
        } else {
            if (ECDSA.recover(digestHash, signature) != signer) {
                revert BadSignature();
            }
        }
    }

Furthermore, the checkSignature function in the EIP1271SignatureUtils will forward an external call to the isValidSignature function in the LombardConsortium contract. This implementation of EIP1271 signature verification through the isValidSignature function in the consortium contract differs significantly from the reference implementation as specified in ERC-1271.

- evm-smart-contracts/contracts/consortium/LombardConsortium.sol

    function isValidSignature(
        bytes32 hash,
        bytes memory signature
    ) external view override returns (bytes4 magicValue) {
        ConsortiumStorage storage $ = _getConsortiumStorage();

        if (ECDSA.recover(hash, signature) != $.thresholdAddr) {
                revert BadSignature();
        }

        return EIP1271SignatureUtils.EIP1271_MAGICVALUE;
    }

It is recommended to enforce ERC1271's specifications, such as returning 0xffffffff instead of reverting the transaction when the signature recovery fails.

Remediation Plan

SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

- contracts/LBTC/LBTC.sol

	pragma solidity ^0.8.19;

- contracts/bascule/Bascule.sol

	pragma solidity ^0.8.20;

- contracts/bascule/interfaces/IBascule.sol

	pragma solidity ^0.8.20;

- contracts/consortium/LombardConsortium.sol

	pragma solidity ^0.8.24;

- contracts/consortium/LombardTimeLock.sol

	pragma solidity ^0.8.24;

- contracts/libs/BitcoinUtils.sol

	pragma solidity ^0.8.19;

- contracts/libs/BridgeDepositCodec.sol

	pragma solidity ^0.8.24;

Specify a precise version of Solidity in your contracts instead of using a wide version range. For example, instead of pragma solidity ^0.8.0;, use pragma solidity 0.8.0;. This ensures that the contract is compiled with a specific version of the Solidity compiler, avoiding potential issues that may arise from changes in newer versions.

Remediation Plan

SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and add them to a special data structure known as “topics” instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

- contracts/LBTC/ILBTC.sol

	    event UnstakeRequest(address indexed fromAddress, bytes scriptPubKey, uint256 amount);

- contracts/LBTC/ILBTC.sol

	    event WithdrawalsEnabled(bool);

- contracts/LBTC/ILBTC.sol

	    event NameAndSymbolChanged(string name, string symbol);

- contracts/LBTC/ILBTC.sol

	    event OutputProcessed(bytes32 indexed transactionId, uint32 indexed index, bytes32 proofHash);

- contracts/LBTC/ILBTC.sol

	    event DepositToBridge(address indexed fromAddress, bytes32 indexed toAddress, bytes32 toContract, bytes32 chainId, uint64 amount);

- contracts/LBTC/ILBTC.sol

	    event DepositAbsoluteCommissionChanged(uint64 newValue, bytes32 indexed toChain);

- contracts/LBTC/ILBTC.sol

	    event DepositRelativeCommissionChanged(uint16 newValue, bytes32 indexed toChain);

- contracts/LBTC/ILBTC.sol

	    event BurnCommissionChanged(uint64 prevValue, uint64 newValue);

- contracts/bascule/Bascule.sol

	  event UpdateValidateThreshold(uint256 oldThreshold, uint256 newThreshold);

- contracts/bascule/Bascule.sol

	  event MaxDepositsUpdated(uint256 numDeposits);

- contracts/bascule/Bascule.sol

	  event DepositsReported(uint256 numDeposits);

- contracts/bascule/Bascule.sol

	  event WithdrawalNotValidated(bytes32 depositID, uint256 withdrawalAmount);

- contracts/bascule/Bascule.sol

	  event WithdrawalValidated(bytes32 depositID, uint256 withdrawalAmount);

- contracts/consortium/LombardConsortium.sol

	    event ThresholdAddrChanged(address prevValue, address newValue);

It is recommended to add the indexed keyword when declaring events, considering the following example:

    event Indexed(
        address indexed from,
        bytes32 indexed id,
        uint indexed value
    );

Remediation Plan

SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.

The current contracts in-scope include several functions that are declared as public but are not invoked internally within the smart contracts. These functions are intended to be called only from external sources.

- contracts/LBTC/LBTC.sol

	    function decimals() public override view virtual returns (uint8) {

- contracts/LBTC/LBTC.sol

	    function name() public override view virtual returns (string memory) {

- contracts/LBTC/LBTC.sol

	    function symbol() public override view virtual returns (string memory) {

- contracts/LBTC/LBTC.sol

	    function getBurnCommission() public view returns (uint64) {

- contracts/bascule/Bascule.sol

	  function pause() public onlyRole(PAUSER_ROLE) {

- contracts/bascule/Bascule.sol

	  function unpause() public onlyRole(PAUSER_ROLE) {

- contracts/bascule/Bascule.sol

	  function updateValidateThreshold(uint256 newThreshold) public whenNotPaused {

- contracts/bascule/Bascule.sol

	  function setMaxDeposits(uint256 aMaxDeposits) public whenNotPaused onlyRole(DEFAULT_ADMIN_ROLE) {

To improve code clarity and optimize gas usage, it is recommended to change the visibility of these functions from public to external.

The external keyword is specifically designed for functions that are meant to be called from outside the contract, and it can result in more efficient code execution. By making this change, you can enhance the readability and performance of the smart contract.

Remediation Plan

SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from Ethereum mainnet, like L2 chains that may not support PUSH0, TSTORE, TLOAD and/or MCOPY, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing immutable contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

Remediation Plan

ACKNOWLEDGED: The Lombard team has acknowledged this issue.

The current implementation includes an internal function that is called only once within the smart contract. Instead of separating this logic into a separate function, consider inlining the logic directly into the calling function. This approach can reduce the number of function calls and improve code readability.

- evm-smart-contracts/contracts/libs/EIP1271SignatureUtils.sol

    function checkSignature(address signer, bytes32 digestHash, bytes memory signature) internal view {

        if (isContract(signer)) {
            if (IERC1271(signer).isValidSignature(digestHash, signature) != EIP1271_MAGICVALUE) {
                revert SignatureVerificationFailed();
            }
        } else {
            if (ECDSA.recover(digestHash, signature) != signer) {
                revert BadSignature();
            }
        }
    }

    function isContract(address addr) internal view returns (bool) {
        return addr.code.length != 0;
    }

Consider embedding the logic of the internal function used only once into the caller function itself. This will enhance code readability and reduce the amount of calls.

Remediation Plan

ACKNOWLEDGED: The Lombard team has acknowledged this issue.

For enhanced gas-efficiency, the default values for variables, such as 0 for uint256 does not have to be initialized.

- evm-smart-contracts/contracts/bascule/Bascule.sol

    for (uint256 i = 0; i < numDeposits; i++) {

Do not initialize variables with their default value.

Remediation Plan

ACKNOWLEDGED: The Lombard team has acknowledged this issue.

Using ++i costs less gas than i++, especially when it's used in for loops. The same for --i and i--, where the first is preferred.

- evm-smart-contracts/contracts/bascule/Bascule.sol

    for (uint256 i = 0; i < numDeposits; i++) {
      bytes32 depositID = depositIDs[i];
      if (depositHistory[depositID]) {
        revert AlreadyReported(depositID);
      }
      depositHistory[depositID] = true;
    }

Use ++i / --i for enhanced gas-efficiency.

Remediation Plan

ACKNOWLEDGED: The Lombard team has acknowledged this issue.