A collection of my personal blockchain security audits.
Reach out to me on Twitter for private audits.
My publications on Substack
| Date | Protocol Name |
|---|---|
| 2024-11 | Undisclosed |
| 2024-10 | Undisclosed |
| Date | Protocol Name |
|---|---|
| 2024-11 | Undisclosed |
| 2024-10 | Undisclosed |
| 2024-10 | Ethena |
| 2024-09 | Interpol |
| 2024-07 | Curio |
| 2024-07 | GroupCoin |
| 2024-06 | Bio |
| 2024-05 | Fyde |
| 2024-05 | Aburra |
| 2024-05 | Rivus |
| 2024-04 | Open Dollar |
| 2024-04 | Curio |
| 2024-03 | Fyde |
| 2024-02 | Tapioca DAO |
| Date | Protocol Name | High | Medium | Low | Rank |
|---|---|---|---|---|---|
| 2024-07 | Grass | 2 | - | - | 8 |
| 2024-04 | Curvance | 4 | - | - | 9 |
| Date | Protocol Name | High | Medium | Low | Non-Critical | Gas | Rank |
|---|---|---|---|---|---|---|---|
| 2024-05 | Renzo | 2 | 3 | - | - | - | 16/122 |
| 2024-02 | UniStaker | - | - | - | - | - | 12/47 |
| 2024-01 | Decent | 2 | 3 | - | - | - | 11/113 |
| 2024-01 | Salty.IO | 1 | 3 | - | - | - | 12/177 |
| 2023-12 | Shell | - | - | - | - | - | 1/22 |
| 2023-10 | Ethena | - | 3 | - | - | - | 1/147 |
| 2023-06 | Stader | - | 2 | - | - | - | 20/75 |
| 2023-05 | Venus | - | 3 | - | - | - | 12/102 |
| 2023-04 | Frankencoin | - | 2 | - | - | - | 52/199 |
| 2023-04 | Caviar | - | 1 | - | - | - | 31/120 |
| 2023-03 | Asymmetry | 2 | 3 | 8 | 4 | - | 17/246 |
| 2023-03 | Wenwin | - | 1 | 6 | 8 | - | 21/93 |
| 2023-02 | Popcorn | 3 | - | 4 | 2 | - | 54/169 |
| 2023-01 | RabbitHole Quest | 1 | 3 | 3 | 6 | - | 26/173 |
| 2023-01 | Ondo Finance | 1 | - | 4 | 2 | - | 6/69 |
| 2023-01 | Biconomy | - | 3 | 5 | 3 | - | 21/105 |
| 2022-11 | Debt DAO | - | 1 | 1 | 5 | 4 | 41/120 |
| 2022-10 | Holograph | - | 1 | 2 | 3 | 2 | 35/144 |
| 2022-10 | 3xcalibur | - | 2 | 1 | 3 | 1 | 26/102 |
| 2022-10 | Inverse Finance | - | 2 | - | - | - | 84/127 |
| 2022-10 | Juicebox | - | - | - | 3 | - | 35/67 |
| 2022-09 | VTVL Protocol | - | 1 | 1 | - | 4 | 79/198 |
| 2022-09 | Frax Finance | - | - | - | - | 8 | 99/133 |
| 2022-09 | Y2k Finance | - | - | - | - | 6 | 102/110 |
| 2022-09 | PartyDAO | - | - | - | - | 2 | 97/110 |
| Date | Protocol Name | High | Medium |
|---|---|---|---|
| 2023-01 | UXD Protocol | 1 | 3 |
| 2022-11 | Buffer Finance | - | 1 |
| 2022-11 | Rage Trade | 1 | - |
| 2022-11 | Astaria | 1 | 2 |
| 2022-10 | Union Finance | - | 1 |
| Date | Protocol Name | Rank |
|---|---|---|
| 2023-02 | Union Finance Update | 4/50 |
| 2023-01 | Ajna | 5/87 |
| 2023-01 | Cooler | 5/68 |
| 2023-01 | UXD Protocol | 10/69 |
| 2023-01 | Illuminate Round 2 | 12/33 |
| 2023-01 | Notional Update | 12/57 |
Top 76th Code4rena 2023 Leaderboards
Warden Name: peanuts
Contest: https://code4rena.com/contests/2023-01-biconomy-smart-contract-wallet-contest
| ID | Title | Severity |
|---|---|---|
| M-01 | handleAggregatedOps() does not handle non-atomic transactions which results in whole function revert if one transaction does not go through | Medium |
| M-02 | EntryPoint address in SmartAccount.sol cannot execute certain functions because of the onlyOwner modifier | Medium |
| M-03 | Protocol has upgradeable contracts that cannot be upgraded | Medium |
| L-01 | Initializers can be frontrunnable | Low |
| L-02 | Contract should adhere to two-step ownership process | Low |
| L-03 | Unused/Empty receive()/fallback() function | Low |
| L-04 | OpenZeppelin's ECDSA is imported but not utilized | Low |
| L-05 | Import exact functions instead of the whole contract itself | Low |
| N-01 | Non-library/interface files should use fixed compiler versions, not floating ones | Info |
| N-02 | Avoid the use of sensitive terms in favour of neutral ones | Info |
| N-03 | Testing coverage is not adequate enough | Info |
[M-01] handleAggregatedOps() does not handle non-atomic transactions which results in whole function revert if one transaction does not go through
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L93 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L385 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L289
Function reverts if one account or paymaster is not validated, which leads to a waste of time and gas.
EntryPoint.UserOpsPerAggregator() takes in an array of opsPerAggregator in its parameter and loops through each struct. In the function, the function _validatePrepayment() is called which validates the account and paymaster from each opsPerAggregator.
function _validatePrepayment(uint256 opIndex, UserOperation calldata userOp, UserOpInfo memory outOpInfo, address aggregator)
private returns (address actualAggregator, uint256 deadline) {
uint256 preGas = gasleft();
MemoryUserOp memory mUserOp = outOpInfo.mUserOp;
_copyUserOpToMemory(userOp, mUserOp);
outOpInfo.userOpHash = getUserOpHash(userOp);
// validate all numeric values in userOp are well below 128 bit, so they can safely be added
// and multiplied without causing overflow
uint256 maxGasValues = mUserOp.preVerificationGas | mUserOp.verificationGasLimit | mUserOp.callGasLimit |
userOp.maxFeePerGas | userOp.maxPriorityFeePerGas;
require(maxGasValues <= type(uint120).max, "AA94 gas values overflow");
uint256 gasUsedByValidateAccountPrepayment;
(uint256 requiredPreFund) = _getRequiredPrefund(mUserOp);
(gasUsedByValidateAccountPrepayment, actualAggregator, deadline) = _validateAccountPrepayment(opIndex, userOp, outOpInfo, aggregator, requiredPreFund);
//a "marker" where account opcode validation is done and paymaster opcode validation is about to start
// (used only by off-chain simulateValidation)
numberMarker();
The function then continues to call _validateAccountPrepayment which does another round of checks.
function _validateAccountPrepayment(uint256 opIndex, UserOperation calldata op, UserOpInfo memory opInfo, address aggregator, uint256 requiredPrefund)
internal returns (uint256 gasUsedByValidateAccountPrepayment, address actualAggregator, uint256 deadline) {
unchecked {
uint256 preGas = gasleft();
MemoryUserOp memory mUserOp = opInfo.mUserOp;
address sender = mUserOp.sender;
_createSenderIfNeeded(opIndex, opInfo, op.initCode);
if (aggregator == SIMULATE_FIND_AGGREGATOR) {
numberMarker();
if (sender.code.length == 0) {
// it would revert anyway. but give a meaningful message
revert FailedOp(0, address(0), "AA20 account not deployed");
}
if (mUserOp.paymaster != address(0) && mUserOp.paymaster.code.length == 0) {
// it would revert anyway. but give a meaningful message
revert FailedOp(0, address(0), "AA30 paymaster not deployed");
}
try IAggregatedAccount(sender).getAggregator() returns (address userOpAggregator) {
aggregator = actualAggregator = userOpAggregator;
} catch {
aggregator = actualAggregator = address(0);
}
}
If any opsPerAggregator is not validated, the whole function will revert, leading to a waste of time and gas.
VSCode
Make the loop non-atomic by using a try/catch block. If the paymaster / account is not validated, skip the loop and validate the next one. In _compensate, make sure to count the collected amount correctly if some opsPerAggregator are skipped because of validation failure.
[M-02] EntryPoint address in SmartAccount.sol cannot execute certain functions because of the onlyOwner modifier
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L460 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L465 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L494
EntryPoint address in SmartAccount.sol cannot execute certain functions because of the onlyOwner modifier.
Proof of Concept The functions SmartAccount.execute() and SmartAccount.executeBatch() has an onlyOwner modifier.
function execute(address dest, uint value, bytes calldata func) external onlyOwner{
function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
However, both functions call another function _requireFromEntryPointOrOwner();
_requireFromEntryPointOrOwner();
which checks if msg.sender is adderss(entryPoint()) or owner
function _requireFromEntryPointOrOwner() internal view {
require(msg.sender == address(entryPoint()) || msg.sender == owner, "account: not Owner or EntryPoint");
}
Since execute() and executeBatch() has an onlyOwner modifier which checks if msg.sender is owner only,
modifier onlyOwner {
require(msg.sender == owner, "Smart Account:: Sender is not authorized");
_;
}
address(entryPoint()) cannot call the function execute() and executeBatch().
VSCode
Either take out the onlyOwner modifer in the two functions or remove the _requireFromEntryPointOrOwner() check if the function intends to not allow the msg.sender to be address(entryPoint()).
Protocol cannot upgrade the upgradeable contract.
SmartAccount.sol uses ReentrancyGuardUpgradeable but not UUPSUpgradeable.
contract SmartAccount is
Singleton,
BaseSmartAccount,
IERC165,
ModuleManager,
SignatureDecoder,
SecuredTokenTransfer,
ISignatureValidatorConstants,
FallbackManager,
Initializable,
ReentrancyGuardUpgradeable
{
Manual Review
import UUPSUpgradeable, inherit the contract, and initialize the contract
import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
contract SmartAccount is
Singleton,
BaseSmartAccount,
IERC165,
ModuleManager,
SignatureDecoder,
SecuredTokenTransfer,
ISignatureValidatorConstants,
FallbackManager,
Initializable,
ReentrancyGuardUpgradeable
+ UUPSUpgradeable
{
function init(address _owner, address _entryPointAddress, address _handler) public override initializer {
require(owner == address(0), "Already initialized");
require(address(_entryPoint) == address(0), "Already initialized");
require(_owner != address(0),"Invalid owner");
require(_entryPointAddress != address(0), "Invalid Entrypoint");
require(_handler != address(0), "Invalid Entrypoint");
+ __UUPSUpgradeable_init();
owner = _owner;
_entryPoint = IEntryPoint(payable(_entryPointAddress));
if (_handler != address(0)) internalSetFallbackHandler(_handler);
setupModules(address(0), bytes(""));
}
Also add storage gap for upgradeable contracts.
uint256[45] private __gap;
If the initializer is not executed in the same transaction as the constructor, a malicious user can front-run the initialize() call, forcing the contract to be redeployed.
Setting the owner within one function can be dangerous if the address of the owner in the parameter is not set properly. Consider using a two-step process whereby the owner has to acknowledge the change before changing the ownership.
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds
Use OpenZeppelin’s ECDSA contract rather than calling ecrecover() directly
Example:
import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; instead of
import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; Applies to all contracts in Biconomy protocol.
SmartAccount.sol:
import "./libs/LibAddress.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; import "./BaseSmartAccount.sol"; import "./common/Singleton.sol"; import "./base/ModuleManager.sol"; import "./base/FallbackManager.sol"; import "./common/SignatureDecoder.sol"; import "./common/SecuredTokenTransfer.sol"; import "./interfaces/ISignatureValidator.sol"; import "./interfaces/IERC165.sol"; import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
eg. 0.8.10 instead of ^0.8.10
Use allowlist rather than whitelist.
What is the overall line coverage percentage provided by your tests?: 41 Protocol should aim to achieve >95% testing coverage to make sure that all functions are in working order.