Prepared by:
HALBORN
Last Updated 08/20/2024
Date of Engagement by: July 23rd, 2024 - August 5th, 2024
100% of all REPORTED Findings have been addressed
All findings
10
Critical
0
High
1
Medium
0
Low
2
Informational
7
Lombard engaged Halborn to conduct a security assessment on their smart contracts beginning on July 23rd, 2024, and ending on August 5th, 2024. The security assessment was scoped to the smart contracts provided in the lombard-finance/evm-smart-contracts GitHub repository. Commit hashes and further details can be found in the Scope section of this report.
Halborn was provided 2 weeks for the engagement, and assigned one full-time security engineer to review the security of the smart contracts in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of the assessment is to:
Identify potential security issues within the smart contracts.
Ensure that smart contract functionality operates as intended.
In summary, Halborn identified some security issues, that were successfully addressed by the Lombard team. The main security issues were:
Denial of Service and Permanent loss of funds.
Centralization risk for trusted owners.
ERC1271 implementation divergences.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the code and can quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the assessment:
Research into architecture and purpose.
Smart contract manual code review and walkthrough.
Graphing out functionality and contract logic/connectivity/functions (solgraph).
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes.
Manual testing by custom scripts.
Static Analysis of security for scoped contract, and imported functions (slither).
Testnet deployment (hardhat).
| EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
| Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
| Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
| IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
| Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
| Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
| SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
|---|---|---|
| Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
| Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
| Severity | Score Value Range |
|---|---|
| Critical | 9 - 10 |
| High | 7 - 8.9 |
| Medium | 4.5 - 6.9 |
| Low | 2 - 4.4 |
| Informational | 0 - 1.9 |
Critical
0
High
1
Medium
0
Low
2
Informational
7
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| Denial of Service and Permanent loss of funds | High | Solved - 08/07/2024 |
| Centralization Risk for trusted owners | Low | Solved - 08/07/2024 |
| ERC1271 implementation divergences | Low | Solved - 08/07/2024 |
| Floating pragma | Informational | Solved - 08/07/2024 |
| Events fields are missing "indexed" attribute | Informational | Solved - 08/07/2024 |
| Change 'public' functions that are not invoked internally to 'external' | Informational | Solved - 08/07/2024 |
| PUSH0 and other opcodes are not supported on all chains | Informational | Acknowledged |
| Internal function used once can be embedded | Informational | Acknowledged |
| Variables initialized with default value | Informational | Acknowledged |
| Iterate with '++i' for enhanced gas-efficiency | Informational | Acknowledged |
// High
The burn function in the LBTC.sol smart contract is designed to burn a specified amount of $LBTC from the msgSender() on the source EVM-chain and transfer the corresponding amount to a destination scriptPubkey on the Bitcoin network. The scriptPubkey can be of type Taproot (P2TR) or Pay-To-Witness (P2WPKH).
The scriptPubkey format is validated through the BitcoinUtils library, specifically using the internal getOutputType function. The amount to be burned and transferred to the caller's provided scriptPubkey on the Bitcoin network is calculated by deducting a fee amount, which is fetched from the burnCommission state variable.
- evm-smart-contracts/contracts/LBTC/LBTC.sol
function burn(bytes calldata scriptPubkey, uint256 amount) external {
OutputType outType = BitcoinUtils.getOutputType(scriptPubkey);
if (outType == OutputType.UNSUPPORTED) {
revert ScriptPubkeyUnsupported();
}
LBTCStorage storage $ = _getLBTCStorage();
if (!$.isWithdrawalsEnabled) {
revert WithdrawalsDisabled();
}
uint64 fee = $.burnCommission;
if (amount <= fee) {
revert AmountLessThanCommission(fee);
}
amount -= fee;
address fromAddress = address(_msgSender());
_transfer(fromAddress, getTreasury(), fee);
_burn(fromAddress, amount);
emit UnstakeRequest(
fromAddress,
scriptPubkey,
amount
);
}However, the burnCommission state variable is not initialized through the constructor, and the provided scripts do not enforce calling the changeBurnCommission function during contract initialization or setup. Consequently, the fee internal variable of the burn function will fetch the default value for the type, which is zero (0).
The issue arises because, in this scenario, no minimum amount is enforced on each burn operation. The burnCommission fee is intended to serve as a security threshold to prevent the burning of small amounts of satoshis, which would result in loss-making transactions and overloaded off-chain processes.
Since the burnCommission state variable is never initialized and thus fee == 0, there are no verifications in place to block a malicious user from intentionally requesting the burn of smallest amounts, such as 1 satoshi. The consequences of not having a minimum amount to burn can lead to critical issues, including:
Denial of Service: The off-chain component will queue all UnstakeRequest event emissions and be flooded with requests for small amounts from the same user, effectively blocking or considerably delaying legitimate requests.
Drain of Funds: The wallet used to sign transactions in off-chain processes will incur gas fees for repetitive small transactions, leading to a drain of funds.
To reproduce this vulnerability, any user with a small amount of LBTC can call the burn function repetitively, passing an extremely low value for amount.
In the following example, an attacker sends 10.000 burn transactions. The burn function is called and all the events are emitted, effectively leading the off-chain components to a flooded state because of missing input validations on-chain.
PoC Code:
describe.only("HAL-10 - Denial of Service and Permanent loss of funds", function () {
beforeEach(async function () {
await snapshot.restore();
await lbtc.toggleWithdrawals();
});
it("Burn smallest amount leads to DOS and funds drainage", async () => {
for(let i = 0; i < 10_000; i++) {
const amount = 1;
const p2wpkh = "0x00143dee6158aac9b40cd766b21a1eb8956e99b10000"; //ff03
await lbtc["mint(address,uint256)"](await signer1.getAddress(), amount);
await expect(lbtc.connect(signer1).burn(p2wpkh, amount))
.to.emit(lbtc, "UnstakeRequest")
.withArgs(await signer1.getAddress(), p2wpkh, amount);
}
});
})![HAL-10[PoC]](https://halbornmainframe.com/proxy/audits/images/66b15ceed69c4486d4df3f67)
Initialize the burnCommission state variable during contract deployment to ensure a non-zero fee is applied.
Enforce a minimum burn amount to prevent the burning of small, non-viable amounts of satoshis.
SOLVED: The Lombard team has solved this issue. The commit hash is 76e6b85a704b9edf5c7ebe9e41675c25a4ce89dd.
// Low
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.
SOLVED: The Lombard team has solved this issue, by enforcing the usage of Safe multi-signature wallets for administrative tasks.
// Low
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.
SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.
// Informational
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.
SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.
// Informational
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
);SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.
// Informational
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.
SOLVED: The Lombard team has solved this issue. The commit hash is e1620001e623079ad1040a806f395460d30e61b6.
// Informational
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.
ACKNOWLEDGED: The Lombard team has acknowledged this issue.
// Informational
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.
ACKNOWLEDGED: The Lombard team has acknowledged this issue.
// Informational
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.
ACKNOWLEDGED: The Lombard team has acknowledged this issue.
// Informational
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.
ACKNOWLEDGED: The Lombard team has acknowledged this issue.
Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their ABIs and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.
The security team assessed all findings identified by the Slither software, however, findings with severity Information and Optimization are not included in the below results for the sake of report readability.
All issues identified by Slither were proved to be false positives, and therefore have not been added to the issue list in this report.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed