Platform: Code4rena
Start Date: 21/12/2023
Pot Size: $90,500 USDC
Total HM: 10
Participants: 39
Period: 18 days
Judge: LSDan
Total Solo HM: 5
Id: 315
League: ETH
Rank: 29/39
Findings: 1
Award: $21.90
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x11singh99, 0xA5DF, 0xMilenov, 0xTheC0der, 7ashraf, Bauchibred, EV_om, Kaysoft, Sathish9098, SpicyMeatball, cheatc0d3, erebus, hash, imare, immeas, joaovwfreire, lil_eth, lsaudit, oakcobalt, para8956, peanuts, rvierdiiev, slvDev, trachev
21.8971 USDC - $21.90
The propose function does not validate the input parameters, which could lead to invalid proposals being created.
function propose( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, string memory description ) public override returns (uint256) { require(targets.length == values.length, "Mismatched targets and values"); require(targets.length == calldatas.length, "Mismatched targets and calldatas"); require(targets.length > 0, "Must provide at least one target"); return super.propose(targets, values, calldatas, description); }
Validating that the lengths of targets, values, and calldatas arrays match ensures that each target has a corresponding value and calldata. Checking that there is at least one target prevents empty proposals.
The contract may allow proposals with insufficient support to be created if the proposal threshold is not enforced.
function propose( // ... parameters ) public override returns (uint256) { require(getVotes(msg.sender, block.number - 1) >= proposalThreshold(), "Proposer votes below threshold"); // Existing logic... }
Checking that the proposer's votes meet the proposal threshold ensures that only those with sufficient support can create proposals, which is a key governance safeguard.
The _execute and _cancel functions are internal and assume that the caller is authorized, but additional checks could be added for extra security.
function _execute( // parameters ) internal override { require(state(proposalId) == ProposalState.Queued, "Proposal must be queued"); } function _cancel( ) internal override { require(state(proposalId) == ProposalState.Pending, "Proposal must be pending"); }
Adding state checks ensures that proposals are in the correct state before execution or cancellation, preventing unauthorized or premature actions.
The propose function allows a user to submit a new proposal. If there are no checks on the frequency or number of proposals a user can make, this could lead to a DoS attack by spamming the governance system with proposals.
// @audit Add a mapping to track the last proposal timestamp for each address mapping(address => uint256) public lastProposalTimestamp; function propose( ) public override returns (uint256) { require(block.timestamp - lastProposalTimestamp[msg.sender] >= proposalCooldown, "Must wait for cooldown period"); lastProposalTimestamp[msg.sender] = block.timestamp; } uint256 public proposalCooldown = 1 weeks; // @audit Cooldown period between proposals from the same address
Introducing a cooldown period between proposals from the same address can prevent spam and ensure that governance resources are used effectively.
There is no enforced delay between when a proposal is created and when it can be executed, which could lead to rushed decisions.
function _execute( // parameters ) internal override { require(block.timestamp >= proposalTimelock[proposalId], "Execution before timelock"); } mapping(uint256 => uint256) public proposalTimelock; function propose( // parameters ) public override returns (uint256) { uint256 proposalId = super.propose(targets, values, calldatas, description); proposalTimelock[proposalId] = block.timestamp + executionDelay; return proposalId; } uint256 public executionDelay = 2 days; // @audit Delay before a proposal can be executed
Adding an execution delay ensures that there is a period for reflection and discussion before a proposal is executed, promoting thoughtful governance.
Ensure that the parameters provided to the _cancel function match the proposal details to prevent cancellation with incorrect data.
function _cancel( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) internal override returns (uint256) { require(targets.length == values.length, "Mismatch between targets and values"); require(targets.length == calldatas.length, "Mismatch between targets and calldatas"); }
Validating the length of the arrays ensures that each target has a corresponding value and calldata, same as _execute function.
Evaluating conditions that are unlikely to be true can waste gas.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/GovernorOLAS.sol#L68
function _execute( uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) internal override { ProposalState currentState = state(proposalId); if (currentState != ProposalState.Queued) { return; // Short-circuit to save gas if the proposal is not in the queued state } super._execute(proposalId, targets, values, calldatas, descriptionHash); }
Using short-circuiting in conditionals can save gas by avoiding unnecessary computation when the condition is likely to be false.
A single account can change the owner or minter, which is a centralization risk.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/OLAS.sol#L43C5-L54C6
// @audit Introduce a multi-signature requirement for changing critical roles function changeOwner(address newOwner) external onlyMultisig { require(newOwner != address(0), "ZeroAddress"); owner = newOwner; emit OwnerUpdated(newOwner); } function changeMinter(address newMinter) external onlyMultisig { require(newMinter != address(0), "ZeroAddress"); minter = newMinter; emit MinterUpdated(newMinter); } // @audit Modifier to check if the caller is a multisig contract modifier onlyMultisig() { require(isMultisig(msg.sender), "Not multisig"); _; }
This change introduces a multi-signature requirement for changing the owner or minter, reducing the risk of a single point of failure.
The use of block.timestamp can be manipulated by miners to a certain degree, which might affect the inflation control mechanism.
function inflationRemainder() public view returns (uint256 remainder) { uint256 _totalSupply = totalSupply; // Current year uint256 numYears = (block.timestamp - timeLaunch) / oneYear; // Calculate maximum mint amount to date uint256 supplyCap = tenYearSupplyCap; // After 10 years, adjust supplyCap according to the yearly inflation % set in maxMintCapFraction if (numYears > 9) { // Number of years after ten years have passed (including ongoing ones) numYears -= 9; for (uint256 i = 0; i < numYears; ++i) { supplyCap += (supplyCap * maxMintCapFraction) / 100; } }
Use a more reliable measure of time passage, such as block numbers, or introduce a tolerance for timestamp manipulation to mitigate the impact of any potential manipulation.
To address potential precision errors in the inflationControl function, we use SafeMath for arithmetic operations.
import "@openzeppelin/contracts/utils/math/SafeMath.sol"; function inflationRemainder() public view returns (uint256 remainder) { uint256 _totalSupply = totalSupply; uint256 numYears = SafeMath.sub(block.timestamp, timeLaunch) / oneYear; uint256 supplyCap = tenYearSupplyCap; if (numYears > 9) { uint256 additionalYears = SafeMath.sub(numYears, 9); for (uint256 i = 0; i < additionalYears; ++i) { supplyCap = SafeMath.add(supplyCap, SafeMath.div(SafeMath.mul(supplyCap, maxMintCapFraction), 100)); } } remainder = SafeMath.sub(supplyCap, _totalSupply); }
Using SafeMath's add, sub, mul, and div functions ensures that arithmetic operations in the inflationRemainder function are performed safely without overflow or underflow, maintaining precision.
The changeOwner and changeMinter functions allow the owner to unilaterally change critical roles in the contract.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/OLAS.sol#L43C5-L54C6
function changeOwner(address newOwner) external { if (msg.sender != owner) { revert ManagerOnly(msg.sender, owner); } // ... }
Implement a multi-signature mechanism or a time-delayed commit-reveal scheme to require multiple confirmations before such critical changes take effect.
The loop in inflationRemainder can lead to high gas costs and could be exploited to cause a denial-of-service attack by repeatedly calling the function with parameters that maximize loop iterations.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/OLAS.sol#L108
for (uint256 i = 0; i < numYears; ++i) { supplyCap += (supplyCap * maxMintCapFraction) / 100; }
Replace the loop with a closed-form formula for compound interest calculation to avoid iteration and reduce gas costs.
The use of integer division in the inflation calculation can lead to rounding errors, potentially causing discrepancies in the supply cap.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/OLAS.sol#L109
supplyCap += (supplyCap * maxMintCapFraction) / 100;
Use a fixed-point arithmetic library or implement a rounding policy to minimize the impact of rounding errors.
Accessing storage variables in loops can be gas-intensive.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/veOLAS.sol#L690
function _supplyLockedAt(PointVoting memory lastPoint, uint64 ts) internal view returns (uint256 vSupply) { // @audit Cache storage variables in memory before the loop int128 slope = lastPoint.slope; int128 bias = lastPoint.bias; uint64 lastTs = lastPoint.ts; // @audit Use cached variables in the loop }
Caching storage variables in memory before a loop and using the cached variables within the loop can save gas.
The contract performs arithmetic operations without explicitly checking for overflows or underflows.
import "@openzeppelin/contracts/utils/math/SafeMath.sol"; function _depositFor( // parameters ) internal { // @audit Use SafeMath for arithmetic operations supply = SafeMath.add(supply, amount); }
Using SafeMath for arithmetic operations ensures that they are performed safely without overflow or underflow, which is especially important for financial contracts.
The increaseUnlockTime function does not validate that the new unlock time is greater than the current unlock time.
function increaseUnlockTime(uint256 unlockTime) external { LockedBalance storage lockedBalance = mapLockedBalances[msg.sender]; uint256 newUnlockTime = ((block.timestamp + unlockTime) / WEEK) * WEEK; require(newUnlockTime > lockedBalance.endTime, "New unlock time must be greater than current unlock time"); // Existing logic... }
This validation ensures that users can only increase their lock time, not decrease it, which is important for maintaining the integrity of the voting power system.
Ensure that the new unlock time is in the future and extends the current lock.
function increaseUnlockTime(uint256 unlockTime) external { LockedBalance storage userLock = mapLockedBalances[msg.sender]; require(unlockTime > userLock.endTime, "New unlock time must be greater than current unlock time"); // Existing logic... }
This validation ensures that users can only extend their lock time, not decrease it, which is important for maintaining the integrity of the voting power system.
Check Effects Interactions Pattern: The contract should follow the checks-effects-interactions pattern to prevent reentrancy issues. This means that state changes should occur before external calls. Before: IERC20(token).transfer(msg.sender, amount); mapLockedBalances[msg.sender] = LockedBalance(0, 0); Recommended: mapLockedBalances[msg.sender] = LockedBalance(0, 0); IERC20(token).transfer(msg.sender, amount);
Updating the state before making the external call ensures that the contract's state is consistent even if the external call leads to a reentrant call back into the contract.
The contract assumes that the transferFrom and transfer functions of the ERC20 token will revert on failure. While this is a common pattern, especially with tokens that follow the ERC20 standard closely, it is not guaranteed by the standard itself.
IERC20(token).transferFrom(msg.sender, address(this), amount); ... IERC20(token).transfer(msg.sender, amount);
bool sent = IERC20(token).transferFrom(msg.sender, address(this), amount); require(sent, "Token transfer failed"); ... sent = IERC20(token).transfer(msg.sender, amount); require(sent, "Token transfer failed");
Checking the return value of the transferFrom and transfer calls and requiring that it is true ensures that the token transfer was successful. This is a safer approach and can prevent subtle bugs if the ERC20 token does not revert on failure.
The contract uses block timestamps for calculating lock periods and voting weights. Block timestamps can be slightly manipulated by miners and are not precise to the second.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/veOLAS.sol#L192
if (newLocked.endTime > block.timestamp && newLocked.amount > 0) { uNew.slope = int128(newLocked.amount) / IMAXTIME; uNew.bias = uNew.slope * int128(uint128(newLocked.endTime - uint64(block.timestamp))); }
Reliance on block timestamps for critical logic can lead to inaccuracies in voting power calculations. In extreme cases, miner manipulation of timestamps could be used to gain an advantage in governance decisions.
The _checkpoint function iterates over a potentially large number of weeks to fill in the history. If the number of iterations is large enough, it could hit the block gas limit, making it impossible to call successfully.
for (uint256 i = 0; i < 255; ++i) { // loop body that potentially runs 255 times }
An attacker could deliberately create conditions that maximize the loop's iterations, causing transactions that call _checkpoint to consistently fail due to out-of-gas errors. This would effectively lock the contract, preventing users from creating, extending, or withdrawing from locks, and could disrupt the governance process.
The contract does not emit events for certain state changes, such as when user balances are updated or when slope changes are recorded. This lack of transparency can make it difficult to track changes and could be exploited to hide malicious activities.
mapLockedBalances[account] = lockedBalance;
Without events logging these changes, it becomes harder for off-chain services and users to verify and react to changes in the contract state. This could be exploited by an attacker to make stealthy changes that are not easily noticed, potentially leading to unnoticed security breaches or manipulation of the voting power.
The contract calculates vote weight based on the amount of tokens locked and the time remaining until the lock expires. The use of integer division can lead to precision loss, especially with large numbers or small differences in timestamps.
uNew.slope = int128(newLocked.amount) / IMAXTIME;
Precision loss in vote weight calculations could lead to discrepancies in the distribution of voting power. This could be exploited by an attacker to gain a disproportionate amount of influence in governance decisions.
The contract uses unchecked type casts, which could lead to unexpected behavior if the casted value does not fit into the target type.
lockedBalance.endTime = uint64(unlockTime);
If unlockTime is larger than what can be represented by uint64, the cast will truncate the value, potentially leading to incorrect lock end times. This could be exploited to create locks that expire prematurely or never expire, affecting the integrity of the voting system.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/veOLAS.sol
The contract assumes that the ERC20 token has 18 decimals, which is standard for many tokens but not guaranteed. This could lead to incorrect calculations of voting power if the token has a different number of decimals.
If the contract interacts with a token that does not have 18 decimals, all calculations related to token amounts and voting power could be incorrect, leading to potential loss of funds or unfair distribution of voting power.
The contract assumes that the transferFrom and transfer functions of the ERC20 token will revert on failure. This assumption may not hold for all ERC20 tokens.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/veOLAS.sol#L364
IERC20(token).transferFrom(msg.sender, address(this), amount); ... IERC20(token).transfer(msg.sender, amount);
Checking the return value of the transferFrom and transfer calls and requiring that it is true ensures that the token transfer was successful. This is a safer approach and can prevent issues if the ERC20 token does not revert on failure.
bool sent = IERC20(token).transferFrom(msg.sender, address(this), amount); require(sent, "Token transfer failed"); ... sent = IERC20(token).transfer(msg.sender, amount); require(sent, "Token transfer failed");
The contract uses storage variables extensively, which can lead to high gas costs.
https://github.com/code-423n4/2023-12-autonolas/blob/main/governance/contracts/veOLAS.sol#L355
mapLockedBalances[account] = lockedBalance;
Using a storage pointer to update multiple fields of a struct in storage can save gas by avoiding multiple SSTORE operations.
LockedBalance storage locked = mapLockedBalances[account]; locked.amount = newAmount; locked.endTime = newEndTime;
Ensure that the timestamp used in voting power calculations is not smaller than the last supply point timestamp to prevent incorrect calculations.
function totalSupplyLockedAtT(uint256 ts) external view returns (uint256 vPower) { uint256 numPoints = IVEOLAS(ve).totalNumPoints(); PointVoting memory sPoint = IVEOLAS(ve).mapSupplyPoints(numPoints - 1); require(ts >= sPoint.ts, "Timestamp is too early"); vPower = IVEOLAS(ve).totalSupplyLockedAtT(ts); }
Adding a check for the timestamp ensures that the voting power calculation uses a valid timestamp, preventing potential errors due to incorrect data.
Ensure that the index provided for supply point retrieval is within bounds to prevent out-of-range errors.
function mapSupplyPoints(uint256 idx) external view returns (PointVoting memory sPoint) { uint256 numPoints = IVEOLAS(ve).totalNumPoints(); require(idx < numPoints, "Index out of bounds"); sPoint = IVEOLAS(ve).mapSupplyPoints(idx); }
This check ensures that the index is within the range of existing supply points, preventing potential errors due to out-of-range access.
When calculating voting power at a specific block number, ensure that the block number provided is not in the future.
function getPastVotes(address account, uint256 blockNumber) external view returns (uint256 balance) { require(blockNumber <= block.number, "Future block number provided"); balance = IVEOLAS(ve).getPastVotes(account, blockNumber); }
This check ensures that the function cannot be called with a block number that has not yet occurred, which would otherwise lead to incorrect or undefined behavior.
Ensure that the timestamp for slope changes is valid and does not exceed the current block timestamp.
function mapSlopeChanges(uint64 ts) external view returns (int128 slopeChange) { require(ts <= uint64(block.timestamp), "Future timestamp provided"); slopeChange = IVEOLAS(ve).mapSlopeChanges(ts); }
This validation ensures that the function cannot be called with a timestamp that is in the future, which would otherwise lead to incorrect or undefined behavior.
When retrieving a user point, ensure that the index is within the range of the user's points to prevent out-of-bounds access.
function getUserPoint(address account, uint256 idx) external view returns (PointVoting memory uPoint) { uint256 userNumPoints = IVEOLAS(ve).getNumUserPoints(account); require(idx < userNumPoints, "Index out of bounds"); uPoint = IVEOLAS(ve).getUserPoint(account, idx); }
This check ensures that the index is within the range of the user's points, preventing potential errors due to out-of-range access.
When locking tokens, ensure that the lock end time is in the future to prevent locking tokens without any voting power benefit.
function lockedEnd(address account) external view returns (uint256 unlockTime) { unlockTime = IVEOLAS(ve).lockedEnd(account); require(unlockTime > block.timestamp, "Lock end time is not in the future"); }
This validation ensures that the lock end time is in the future, which is necessary for the tokens to have voting power and for the lock to have a purpose.
The getUserPoint function retrieves the number of user points to check if the user has any points before attempting to get a specific point. This can be optimized by directly attempting to fetch the user point and relying on the underlying veOLAS contract to revert if the index is out of bounds.
function getUserPoint(address account, uint256 idx) public view returns (PointVoting memory uPoint) { uint256 userNumPoints = IVEOLAS(ve).getNumUserPoints(account); if (userNumPoints > 0) { uPoint = IVEOLAS(ve).getUserPoint(account, idx); } }
change to:
function getUserPoint(address account, uint256 idx) public view returns (PointVoting memory uPoint) { uPoint = IVEOLAS(ve).getUserPoint(account, idx); }
The getNumUserPoints call is unnecessary because the underlying veOLAS contract will revert if the index is out of bounds. This saves gas by eliminating an extra external call when the index is valid.
Adding a check for the existence of supply points and using numPoints - 1 to access the last supply point ensures that we do not attempt to access an index that does not exist. This also prevents a potential underflow if numPoints were to be zero.
function totalSupplyLockedAtT(uint256 ts) external view returns (uint256 vPower) { uint256 numPoints = IVEOLAS(ve).totalNumPoints(); PointVoting memory sPoint = IVEOLAS(ve).mapSupplyPoints(numPoints); if (ts >= sPoint.ts) { vPower = IVEOLAS(ve).totalSupplyLockedAtT(ts); } else { revert WrongTimestamp(sPoint.ts, ts); } }
function totalSupplyLockedAtT(uint256 ts) external view returns (uint256 vPower) { uint256 numPoints = IVEOLAS(ve).totalNumPoints(); if (numPoints == 0) { revert NoSupplyPoints(); } PointVoting memory sPoint = IVEOLAS(ve).mapSupplyPoints(numPoints - 1); if (ts < sPoint.ts) { revert WrongTimestamp(sPoint.ts, ts); } vPower = IVEOLAS(ve).totalSupplyLockedAtT(ts); }
Check that the chain Id provided is supported and matches the expected value for the target and selector.
function _verifyData(address target, bytes memory data, uint256 chainId) internal { require(chainId == block.chainid, "Chain Id does not match current chain"); // Existing logic... }
This validation ensures that the data is verified against the correct chain Id, preventing cross-chain vulnerabilities and ensuring that governance actions are executed in the correct context.
Ensure that only authorized combinations of target addresses and function selectors are allowed.
function setTargetSelectorChainIds( address[] memory targets, bytes4[] memory selectors, uint256[] memory chainIds, bool[] memory statuses ) external { require(msg.sender == owner, "OwnerOnly"); // Existing logic... for (uint256 i = 0; i < targets.length; ++i) { require(targets[i] != address(0), "Target address cannot be zero"); require(selectors[i] != bytes4(0), "Selector cannot be zero"); // Existing logic... } }
This check ensures that only authorized combinations of target addresses and function selectors are allowed, preventing unauthorized governance actions.
Ensure that the bridged data is for the correct chain Id and that the bridge mediator addresses match.
function _verifyBridgedData(bytes memory data, uint256 chainId) internal { require(chainId == block.chainid, "Chain Id does not match current chain"); }
This validation ensures that the bridged data is processed for the correct chain Id, preventing cross-chain vulnerabilities and ensuring that governance actions are executed in the correct context.
Ensure that each L1 bridge mediator is mapped to a unique L2 bridge mediator and chain ID to prevent misconfiguration.
function setBridgeMediatorChainIds( address[] memory bridgeMediatorL1s, address[] memory bridgeMediatorL2s, uint256[] memory chainIds ) external { require(msg.sender == owner, "OwnerOnly"); for (uint256 i = 0; i < bridgeMediatorL1s.length; ++i) { require(mapBridgeMediatorL1L2ChainIds[bridgeMediatorL1s[i]] == 0, "Bridge mediator already set"); } }
This check ensures that each L1 bridge mediator is uniquely mapped, preventing potential errors in cross-chain communication.
Ensure that the governor's state is checked against a valid proposal ID to prevent misuse of the pause function.
function pause() external { require(msg.sender == owner || msg.sender == multisig, "Not authorized to pause"); if (msg.sender == multisig) { ProposalState state = IGovernor(governor).state(governorCheckProposalId); require(state == ProposalState.Defeated, "Governor proposal not defeated"); } paused = 2; emit GuardPaused(msg.sender); }
This validation ensures that the multisig can only pause the guard if the governance is considered inactive, based on a defeated proposal, maintaining the integrity of the governance process.
Ensure that the lengths of input arrays are consistent to prevent logic errors in setting target selectors and chain IDs.
function setTargetSelectorChainIds( address[] memory targets, bytes4[] memory selectors, uint256[] memory chainIds, bool[] memory statuses ) external { require(msg.sender == owner, "OwnerOnly"); require(targets.length == selectors.length && targets.length == chainIds.length && targets.length == statuses.length, "Array lengths mismatch"); // Existing logic... }
This check ensures that the lengths of all input arrays are consistent, preventing potential errors when authorizing target-selector-chainId combinations.
Ensure that the data length for bridged data is sufficient to contain all necessary information.
function _verifyBridgedData(bytes memory data, uint256 chainId) internal { require(data.length >= MIN_BRIDGE_DATA_LENGTH, "Bridged data length insufficient"); // Existing logic... }
This validation ensures that the bridged data contains all the necessary information for processing, preventing errors due to insufficient data length.
The contract constructs a single uint256 key for the mapAllowedTargetSelectorChainIds mapping by combining an address, a selector, and a chain ID. This approach is error-prone and could lead to key collisions or make it difficult to manage the keys.
uint256 targetSelectorChainId = uint256(uint160(target)); targetSelectorChainId |= uint256(uint32(bytes4(data))) << 160; targetSelectorChainId |= chainId << 192;
Mitigation:
Use a struct to represent the combined values and map it to the desired value instead of combining them into a single uint256. This will make the code more readable and reduce the risk of key collisions.
struct TargetSelectorChainId { address target; bytes4 selector; uint64 chainId; } mapping(TargetSelectorChainId => bool) public mapAllowedTargetSelectorChainIds;
Ensure that the message sender is the authorized FxChild address when processing messages from the root.
function processMessageFromRoot(uint256 stateId, address rootMessageSender, bytes memory data) external override { require(msg.sender == fxChild, "Caller is not the FxChild"); // Existing logic... }
This check ensures that only messages sent by the FxChild contract are processed, preventing unauthorized entities from triggering the message processing function.
Ensure that the root message sender is the authorized RootGovernor address when processing messages from the root.
function processMessageFromRoot(uint256 stateId, address rootMessageSender, bytes memory data) external override { require(rootMessageSender == rootGovernor, "Caller is not the RootGovernor"); // Existing logic... }
This check ensures that only messages sent by the RootGovernor are processed, maintaining the integrity of cross-chain governance actions.
Emit an event when the rootGovernor is changed for better transparency and traceability.
event RootGovernorChanged(address indexed previousGovernor, address indexed newGovernor); function changeRootGovernor(address newRootGovernor) external { // existing checks emit RootGovernorChanged(rootGovernor, newRootGovernor); rootGovernor = newRootGovernor; // existing code }
Use function modifiers to encapsulate repetitive checks, improving code readability and maintainability.
modifier onlyFxChild() { if (msg.sender != fxChild) { revert FxChildOnly(msg.sender, fxChild); } _; } modifier onlyRootGovernor(address rootMessageSender) { if (rootMessageSender != rootGovernor) { revert RootGovernorOnly(rootMessageSender, rootGovernor); } _; } function processMessageFromRoot(uint256 stateId, address rootMessageSender, bytes memory data) external override onlyFxChild onlyRootGovernor(rootMessageSender) noReentrancy { // existing code }
The processMessageFromRoot function processes an array of transactions in a loop without a limit on the number of transactions, which could lead to out-of-gas errors if the message contains too many transactions.
for (uint256 i = 0; i < dataLength;) { // existing code }
Optimization:
uint256 public constant MAX_TRANSACTIONS_PER_CALL = 10; // Example value uint256 transactionCount = 0; for (uint256 i = 0; i < dataLength && transactionCount < MAX_TRANSACTIONS_PER_CALL;) { // existing code transactionCount++; }
Introducing a limit on the number of transactions processed in a single call prevents potential out-of-gas errors due to too many transactions.
The contract sends Ether using a low-level call, which is prone to reentrancy attacks and does not limit the gas sent along with the Ether.
(bool success, ) = target.call{value: value}(payload);
Optimization:
(bool success, ) = target.call{value: value, gas: MAX_GAS_PER_TRANSACTION}(payload); require(success, "ETH transfer failed");
Limiting the gas sent with the Ether transfer reduces the risk of reentrancy attacks and ensure that the contract does not run out of gas during the transfer.
Check that the message sender is the authorized AMBContractProxyHome address when processing messages from the foreign network.
function processMessageFromForeign(bytes memory data) external { require(msg.sender == AMBContractProxyHome, "Caller is not the AMBContractProxyHome"); }
This check ensures that only messages sent by the AMBContractProxyHome contract are processed, preventing unauthorized entities from triggering the message processing function.
Check that the foreign governor address is the authorized sender when processing messages from the foreign network.
function processMessageFromForeign(bytes memory data) external { address bridgeGovernor = IAMB(AMBContractProxyHome).messageSender(); require(bridgeGovernor == foreignGovernor, "Caller is not the ForeignGovernor"); // Existing logic... }
This check ensures that only messages sent by the ForeignGovernor are processed, maintaining the integrity of cross-chain governance actions.
Check that the data length is sufficient to contain all necessary information for message processing.
function processMessageFromForeign(bytes memory data) external { require(data.length >= DEFAULT_DATA_LENGTH, "Data length below required minimum"); // Existing logic... }
This validation ensures that the data contains all the necessary information for processing, preventing errors due to insufficient data length.
Ensure that the contract reverts if the execution of the target call fails.
function processMessageFromForeign(bytes memory data) external { // Unpack data... (bool success, ) = target.call{value: value}(payload); require(success, "Execution of target call failed"); }
This check ensures that if the target execution fails, the entire transaction is reverted, maintaining the atomicity of operations and preventing partial state changes
Ensure that the payload length does not exceed the remaining data length to prevent buffer overflows.
// Check that the payload length does not exceed the remaining data length. if (i + payloadLength > dataLength) { revert IncorrectDataLength(dataLength - i, payloadLength); }
Emit events after state changes are finalized to follow the best practice of logging the final state.
// Emit the MessageReceived event after all state changes and interactions. emit MessageReceived(governor, data);
The contract uses inline assembly for data unpacking, which is less readable and more error-prone than using Solidity's built-in functions.
// @audit Use Solidity's built-in abi.decode for better readability and safety. (address target, uint96 value, uint32 payloadLength) = abi.decode(data, (address, uint96, uint32));
Using abi.decode improves code readability and reduces the risk of errors associated with inline assembly.
The contract makes external calls using the low-level call function, which can be risky if the target contract is malicious or has fallback functions that can cause unexpected behavior.
(bool success, ) = target.call{value: value}(payload); if (!success) { revert TargetExecFailed(target, value, payload); }
Use a try-catch block to handle failed calls more gracefully.
The contract's mint and burn functions are controlled by a single owner address, which introduces a central point of failure or abuse.
function mint(address account, uint256 amount) external { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } // ... }
Implement a decentralized control mechanism, such as a multisig or a DAO, to manage minting and burning.
The contract uses a simple owner check for privileged functions. This could be improved by using a modifier for reusability and clarity.
function changeOwner(address newOwner) external { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } // ... }
modifier onlyOwner() { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } _; } function changeOwner(address newOwner) external onlyOwner { // ... }
Using a modifier for owner checks reduces code duplication and makes the contract more readable.
The zero address check is repeated in multiple functions. This can also be moved to a modifier for better code reuse.
function changeOwner(address newOwner) external { if (newOwner == address(0)) { revert ZeroAddress(); } // ... }
change:
modifier notZeroAddress(address _address) { if (_address == address(0)) { revert ZeroAddress(); } _; } function changeOwner(address newOwner) external onlyOwner notZeroAddress(newOwner) { // ... }
A modifier centralizes the zero address check, making the code cleaner and reducing the chance of missing this check in future functions.
A malicious actor could front-run the changeOwner transaction to change the owner to an address they control, potentially causing a DoS.
function changeOwner(address newOwner) external onlyOwner notZeroAddress(newOwner) { owner = newOwner; emit OwnerUpdated(newOwner); }
Mitigation:
Implement a commit-reveal scheme or a time lock to prevent front-running.
The deposit and depositTo functions do not have explicit checks to ensure that only authorized entities (like the bridge mediator) can call them. This could potentially allow unauthorized minting of tokens on L1 if the L2 contract is not properly secured.
function deposit(uint256 amount) external { _deposit(msg.sender, amount); }
Implement authorization checks to ensure that only the bridge mediator can initiate deposits.
Events should be emitted after state changes to follow the checks-effects-interactions pattern.
// Deposit tokens on an L2 bridge contract (lock) bool success = IERC20(childToken).transferFrom(msg.sender, address(this), amount); if (!success) { revert TransferFailed(childToken, msg.sender, address(this), amount); } // Emit event after state change emit FxDepositERC20(childToken, rootToken, msg.sender, to, amount);
Emitting events after state changes ensures that the log reflects the final state, which is important for off-chain services and block explorers that rely on event logs.
For better indexing and searching, it's recommended to order event arguments from the most to the least likely to be used as filter parameters.
event FxDepositERC20(address indexed from, address indexed to, address childToken, address rootToken, uint256 amount);
This ordering allows for more efficient filtering by the most relevant parameters, such as the sender and recipient addresses.
// @audit At the top of the contract, import OpenZeppelin's SafeERC20 library import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; // @audit Use SafeERC20 for IERC20 using SafeERC20 for IERC20; // @audit Use safeTransferFrom and safeTransfer in the _withdraw function function _withdraw(address to, uint256 amount) internal { // ... IERC20(rootToken).safeTransferFrom(msg.sender, address(this), amount); // ... IERC20(rootToken).safeTransfer(to, amount); }
The SafeERC20 library provides wrappers around ERC20 operations that throw on failure, which is safer than using raw calls. This helps to prevent issues when interacting with poorly implemented ERC20 tokens.
Follow the Checks-Effects-Interactions pattern to prevent reordering of operations that could lead to vulnerabilities.
function _withdraw(address to, uint256 amount) internal { // Checks if (amount == 0) { revert ZeroValue(); } // Effects IERC20(rootToken).burn(amount); // Interactions bytes memory message = abi.encode(msg.sender, to, amount); _sendMessageToChild(message); bool success = IERC20(rootToken).transferFrom(msg.sender, address(this), amount); if (!success) { revert TransferFailed(rootToken, msg.sender, address(this), amount); } emit FxWithdrawERC20(rootToken, childToken, msg.sender, to, amount); }
Ensuring that all checks and state changes (effects) are done before external calls (interactions) is good practice and can prevent reentrancy and state inconsistencies.
Ensure that state changes are valid and expected, especially after external calls.
function _processMessageFromChild(bytes memory message) internal override { // ... uint256 balanceBefore = IERC20(rootToken).balanceOf(to); IERC20(rootToken).mint(to, amount); uint256 balanceAfter = IERC20(rootToken).balanceOf(to); require(balanceAfter - balanceBefore == amount, "Minting did not occur as expected"); // ... }
Checking the token balance before and after minting ensures that the minting process had the expected effect on the token balance.
The contract assumes that the transferFrom function of the IERC20 token will always succeed. If these functions fail (due to a malicious token contract or other issues), it could lead to a DoS condition.
bool success = IERC20(rootToken).transferFrom(msg.sender, address(this), amount); if (!success) { revert TransferFailed(rootToken, msg.sender, address(this), amount); }
IERC20(rootToken).safeTransferFrom(msg.sender, address(this), amount);
Using the safeTransferFrom function from OpenZeppelin's SafeERC20 library ensures that these operations revert only if the transfer fails. This prevents a malicious or faulty token contract from causing a DoS.
The contract's administrative functions such as changeOwner, changeManager, and setBaseURI are controlled by a single owner address, which introduces a central point of failure or abuse.
function changeOwner(address newOwner) external virtual { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } // ... }
Consider implementing a decentralized control mechanism, such as a multisig or a DAO, to manage these administrative functions.
The tokenByIndex function assumes that the id provided is valid and does not perform any checks to ensure that the resulting unitId is within the valid range of existing tokens.
function tokenByIndex(uint256 id) external view virtual returns (uint256 unitId) { unitId = id + 1; if (unitId > totalSupply) { revert Overflow(unitId, totalSupply); } }
Add input validation to ensure that the id provided will result in a valid unitId.
function tokenByIndex(uint256 id) external view virtual returns (uint256 unitId) { require(id < totalSupply, "Invalid id"); unitId = id + 1; }
Ensure that the unitId provided to the tokenURI function corresponds to an existing token.
function tokenURI(uint256 unitId) public view virtual override returns (string memory) { // ... }
Change:
function tokenURI(uint256 unitId) public view virtual override returns (string memory) { require(_exists(unitId), "ERC721Metadata: URI query for nonexistent token"); // ... }
Ensure that the new base URI is not empty.
function setBaseURI(string memory bURI) external virtual { require(bytes(bURI).length > 0, "GenericRegistry: Base URI cannot be empty"); // ... }
The _getUnitHash function is meant to be called with existing token IDs, ensure that the unitId is valid.
function _getUnitHash(uint256 unitId) internal view virtual returns (bytes32);
function _getUnitHash(uint256 unitId) internal view virtual returns (bytes32) { require(_exists(unitId), "GenericRegistry: Hash query for nonexistent token"); // ... }
Implement a role-based access control mechanism, such as OpenZeppelin's AccessControl, to manage the manager role more securely.
import "@openzeppelin/contracts/access/AccessControl.sol"; contract UnitRegistry is GenericRegistry, AccessControl { // @audit Define a manager role bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE"); constructor(UnitType _unitType) { _setupRole(MANAGER_ROLE, msg.sender); } function create(...) external virtual returns (uint256 unitId) { // Check for the manager privilege for a unit creation require(hasRole(MANAGER_ROLE, msg.sender), "Caller is not a manager"); // ... } }
Using a role-based access control system provides flexibility and security in managing permissions.
Use custom errors instead of revert strings to save gas and provide clearer error messages.
error ManagerOnly(address caller); error ZeroAddress(); error ZeroValue(); // ... function create(...) external virtual returns (uint256 unitId) { // ... if (manager != msg.sender) { revert ManagerOnly(msg.sender); } // ... }
Custom errors are more gas-efficient than revert strings and can carry additional information.
Ensure that events are emitted after all state changes to prevent any inconsistencies between emitted events and contract state.
function create(...) external virtual returns (uint256 unitId) { // ... _safeMint(unitOwner, unitId); emit CreateUnit(unitId, unitType, unitHash); // ... }
Emitting events after state changes ensures that the event data reflects the final state.
The _calculateSubComponents function contains loops that could potentially iterate over a large number of elements, leading to high gas costs and possible DoS if the transaction exceeds the block gas limit.
for (uint32 i = 0; i < numUnits; ++i) { maxNumComponents += numComponents[i]; } for (counter = 0; counter < maxNumComponents; ++counter) { }
After:
// Introduce a constant to limit the maximum number of dependencies uint32 public constant MAX_DEPENDENCIES = 10; function _calculateSubComponents(...) internal view virtual returns (uint32[] memory subComponentIds) { require(unitIds.length <= MAX_DEPENDENCIES, "Exceeds maximum dependencies"); // ... }
Introducing a maximum limit on dependencies can prevent unbounded loops and ensure that the function does not consume excessive gas.
The updateHash function uses an unchecked push operation on an array, which could grow indefinitely and make the function expensive to call.
https://github.com/code-423n4/2023-12-autonolas/blob/main/registries/contracts/UnitRegistry.sol#L121
function updateHash(...) external virtual returns (bool success) { // ... mapUnitIdHashes[unitId].push(unitHash); // ... }
After:
// Introduce a constant to limit the number of hash updates uint256 public constant MAX_HASH_UPDATES = 10; function updateHash(...) external virtual returns (bool success) { require(mapUnitIdHashes[unitId].length < MAX_HASH_UPDATES, "Exceeds max hash updates"); mapUnitIdHashes[unitId].push(unitHash); // ... }
Limiting the number of times a unit's hash can be updated prevents the array from growing indefinitely and ensure that the gas cost remains manageable.
The create function allows for an unbounded number of subcomponents to be added to mapSubComponents.
https://github.com/code-423n4/2023-12-autonolas/blob/main/registries/contracts/UnitRegistry.sol#L49
function create(...) external virtual returns (uint256 unitId) { // ... mapSubComponents[unitId] = subComponentIds; // ... }
After:
// @audit Introduce a constant to limit the number of subcomponents uint256 public constant MAX_SUBCOMPONENTS = 50; function create(...) external virtual returns (uint256 unitId) { // ... require(subComponentIds.length <= MAX_SUBCOMPONENTS, "Exceeds max subcomponents"); mapSubComponents[unitId] = subComponentIds; // ... }
By limiting the number of subcomponents, we prevent the mapSubComponents mapping from becoming too large to iterate over, which could lead to excessive gas costs.
The getLocalSubComponents function could potentially return a very large array, which could cause the function to fail due to out-of-gas errors.
https://github.com/code-423n4/2023-12-autonolas/blob/main/registries/contracts/UnitRegistry.sol#L182
function getLocalSubComponents(uint256 unitId) external view returns (uint32[] memory subComponentIds, uint256 numSubComponents) { // ... }
Ensuring that the number of subcomponents is limited as mentioned earlier will prevent this function from running out of gas.
The _calculateSubComponents function contains loops that could iterate over a large number of elements, potentially leading to high gas costs and DoS.
for (uint32 i = 0; i < numUnits; ++i) { // ... maxNumComponents += numComponents[i]; } // ... for (counter = 0; counter < maxNumComponents; ++counter) { // ... }
Optimization:
uint32 public constant MAX_DEPENDENCIES = 10; function _calculateSubComponents(...) internal view virtual returns (uint32[] memory subComponentIds) { require(unitIds.length <= MAX_DEPENDENCIES, "Exceeds maximum dependencies"); // ... }
Introducing a maximum limit on dependencies prevents unbounded loops and ensures that the function does not consume excessive gas.
The _checkDependencies function iterates over an array without any bounds on its size. This could be exploited by a malicious actor to create a Denial of Service (DoS) attack by submitting a very large array of dependencies, potentially causing the function to run out of gas and fail.
function _checkDependencies(uint32[] memory dependencies, uint32 maxComponentId) internal virtual override { uint32 lastId; for (uint256 iDep = 0; iDep < dependencies.length; ++iDep) { if (dependencies[iDep] < (lastId + 1) || dependencies[iDep] > maxComponentId) { revert ComponentNotFound(dependencies[iDep]); } lastId = dependencies[iDep]; } }
Mitigation:
Implement a limit on the size of the dependencies array to prevent excessively large arrays from being processed.
The loop iterates over the dependencies array without any constraints on its length. An attacker could submit a very large dependencies array, which could cause the transaction to run out of gas and revert, leading to a DoS condition. This is especially problematic if the function is part of a critical workflow, such as registering new agents.
for (uint256 iDep = 0; iDep < dependencies.length; ++iDep) { if (dependencies[iDep] < (lastId + 1) || dependencies[iDep] > componentTotalSupply) { revert ComponentNotFound(dependencies[iDep]); } lastId = dependencies[iDep]; }
Mitigation:
Implement a limit on the maximum number of dependencies that can be processed in a single transaction. This would prevent an attacker from submitting excessively large arrays that could exhaust the gas limit.
The calculateSubComponents function does not perform any validation on the input unitIds. If the function is used in a critical process, an attacker could pass an array with invalid or maliciously crafted unitIds that could lead to incorrect calculations or excessive gas consumption.
https://github.com/code-423n4/2023-12-autonolas/blob/main/registries/contracts/AgentRegistry.sol#L70
function calculateSubComponents(uint32[] memory unitIds) external view returns (uint32[] memory subComponentIds) { subComponentIds = _calculateSubComponents(UnitType.Agent, unitIds); }
Mitigation:
Implement input validation checks to ensure that the unitIds passed to the function are within an acceptable range and conform to expected formats or constraints. This could include checking for duplicates, ensuring the IDs exist, or enforcing a maximum array size.
The function allows the current owner to transfer ownership to a new address. However, if the current owner's account is compromised, the attacker can transfer ownership to their address, gaining full control over the contract.
function changeOwner(address newOwner) external virtual { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } if (newOwner == address(0)) { revert ZeroAddress(); } owner = newOwner; emit OwnerUpdated(newOwner); }
The changeOwner function lacks a mechanism to prevent an attacker from immediately taking over the contract if they gain access to the owner's private key. Once the ownership is transferred, the attacker can pause the contract, preventing legitimate users from interacting with it, or make other unauthorized changes.
Mitigation:
Implement a two-step ownership transfer process.
The lack of checks in the pause and unpause functions lead to redundant calls that do not change the state of the contract.
function pause() external virtual { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } paused = true; emit Pause(msg.sender); } function unpause() external virtual { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } paused = false; emit Unpause(msg.sender); }
The pause and unpause functions do not check whether the contract is already in the desired state (paused or unpaused). This means that if the contract is already paused, calling pause again will still emit an event and use gas, even though the state of the contract does not change. The same applies to the unpause function. Malicious actors could exploit this by repeatedly calling these functions to drain funds from the contract owner's address through gas fees.
Mitigation:
Add a check to ensure that the contract's state is only changed when necessary, and prevent unnecessary gas expenditure and event emissions.
function pause() external virtual { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } if (paused) { revert AlreadyPaused(); } paused = true; emit Pause(msg.sender); } function unpause() external virtual { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } if (!paused) { revert NotPaused(); } paused = false; emit Unpause(msg.sender); }
The create and updateHash functions do not have any access control mechanisms in place. This means that any user can call these functions and potentially create or update components or agents without proper authorization.
function create(...) external returns (uint256 unitId) { ... } function updateHash(...) external returns (bool success) { ... }
After:
modifier onlyOwner() { require(msg.sender == owner, "Not owner"); _; } function create(...) external onlyOwner returns (uint256 unitId) { ... } function updateHash(...) external onlyOwner returns (bool success) { ... }
Adding an onlyOwner modifier ensures that only the owner of the contract can create or update components or agents, providing a basic level of access control.
The contract makes external calls to IRegistry without checking the return value for the create function. It's important to handle the possibility of these calls failing.
unitId = IRegistry(componentRegistry).create(unitOwner, unitHash, dependencies);
(bool success, bytes memory data) = componentRegistry.call( abi.encodeWithSelector( IRegistry.create.selector, unitOwner, unitHash, dependencies ) ); require(success && data.length > 0, "External call to create failed"); unitId = abi.decode(data, (uint256));
Using a low-level call with proper checks ensures that the external call to create is successful and the return value is valid.
The contract has an owner state variable that is set to msg.sender in the constructor, but there is no functionality to transfer ownership or to renounce ownership. This limits the flexibility and future governance of the contract.
constructor(address _componentRegistry, address _agentRegistry) { // ... owner = msg.sender; }
changes:
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); function transferOwnership(address newOwner) public onlyOwner { require(newOwner != address(0), "New owner is the zero address"); emit OwnershipTransferred(owner, newOwner); owner = newOwner; }
Adding an OwnershipTransferred event and a transferOwnership function allows the current owner to transfer control of the contract to a new owner, improving governance.
The contract does not emit events for critical actions such as the creation or update of components or agents. Events are important for off-chain clients to track the state changes of the contract.
event ComponentCreated(uint256 indexed unitId, address indexed unitOwner, bytes32 unitHash, uint32[] dependencies); event HashUpdated(IRegistry.UnitType indexed unitType, uint256 indexed unitId, bytes32 unitHash); // Emit events in the respective functions emit ComponentCreated(unitId, unitOwner, unitHash, dependencies); emit HashUpdated(unitType, unitId, unitHash);
Emitting events after creating a component or updating a hash allows off-chain clients to monitor these actions and react accordingly.
The contract makes an external call to the IGnosisSafeProxyFactory without proper checks to ensure that the call was successful. If the call to createProxyWithNonce fails, the returned address could be the zero address, which would indicate that the proxy contract was not created successfully. However, the current implementation does not handle this scenario, and the multisig variable would be set to the zero address, potentially leading to loss of funds or other unintended behavior.
multisig = IGnosisSafeProxyFactory(gnosisSafeProxyFactory).createProxyWithNonce(gnosisSafe, safeParams, nonce);
Mitigation:
Check Return Value and Use require
To mitigate this issue, the contract should check the return value of the external call and use a require statement to ensure that the returned address is not the zero address. This would prevent the multisig variable from being set to an invalid address and provide a clear error message if the proxy creation fails.
Change:
address proxy = IGnosisSafeProxyFactory(gnosisSafeProxyFactory).createProxyWithNonce(gnosisSafe, safeParams, nonce); require(proxy != address(0), "Proxy creation failed"); multisig = proxy;
The contract manually copies bytes from data to payload in a loop. This can be optimized using Solidity's built-in functions.
bytes memory payload = new bytes(payloadLength); for (uint256 i = 0; i < payloadLength; ++i) { payload[i] = data[i + DEFAULT_DATA_LENGTH]; }
After:
bytes memory payload = new bytes(payloadLength); assembly { extcodecopy(multisig, add(payload, 32), DEFAULT_DATA_LENGTH, payloadLength) }
Using extcodecopy in assembly can copy the bytecode directly from the address, which is more gas-efficient than a manual loop in Solidity.
(bool success, ) = multisig.call(payload); if (!success) { revert MultisigExecFailed(multisig); }
In the snippet above, the contract sends an arbitrary payload to the multisig address using a low-level call. This is risky because:
Reentrancy: If the multisig contract is malicious or has a vulnerability, it could call back into the current contract before the initial operation is completed. This could lead to unexpected behavior, especially if the state is changed after the call. Gas Stipends: The .call method forwards all available gas by default, which can lead to out-of-gas errors if the called contract consumes a lot of gas. Lack of Return Data: The low-level call does not return any data, which means that if the multisig contract function called returns important information, it will not be accessible.
Mitigation: Use Direct Function Calls To mitigate this issue, the contract should use direct function calls to known interfaces instead of low-level calls. This ensures that only the intended function can be called, and it's clearer what the expected behavior is. Additionally, direct function calls automatically check for the success of the call and revert if the call fails, which simplifies error handling.
// Assuming IGnosisSafe interface has the execTransaction function IGnosisSafe gnosisSafe = IGnosisSafe(multisig); (bool success, ) = gnosisSafe.execTransaction(payload); if (!success) { revert MultisigExecFailed(multisig); }
By using the interface's execTransaction function directly, the contract benefits from the Solidity compiler's checks and reverts on failure. This approach also allows the contract to handle any return values from the function call, which can be useful for further validation or logic.
function create( address[] memory owners, uint256 threshold, bytes memory data ) external returns (address multisig) { // ... assembly { multisig := mload(add(data, DEFAULT_DATA_LENGTH)) } // ... }
The contract uses inline assembly to load the multisig address from the data parameter. The issues with this approach are:
Data Integrity: The contract assumes that the data parameter is correctly formatted and that the first 20 bytes indeed represent a valid address. If the data is malformed or intentionally crafted to mislead, it could lead to incorrect behavior or security vulnerabilities.
Validation: There is no validation on the data parameter beyond checking its length. Malicious actors could potentially pass in data that meets the length requirement but contains a malicious address or payload.
Validate and Decode Data Properly To mitigate this issue, the contract should validate and decode the data parameter using Solidity's built-in functions, which are designed to handle such operations safely.
function create( address[] memory owners, uint256 threshold, bytes memory data ) external returns (address multisig) { if (data.length < DEFAULT_DATA_LENGTH) { revert IncorrectDataLength(DEFAULT_DATA_LENGTH, data.length); } // @audit Decode the address from the data parameter multisig = abi.decode(data[:DEFAULT_DATA_LENGTH], (address)); }
Using abi.decode ensures the contract can safely extract the address from the data parameter and ensure that it is properly formatted. This approach also provides better readability and maintainability of the code.
The deposit function makes an external call to ITreasury(treasury).depositTokenForOLAS, which could fail and revert the entire transaction.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol
function deposit(uint256 productId, uint256 tokenAmount) external returns (uint256 payout, uint256 maturity, uint256 bondId) { // ... existing code ... ITreasury(treasury).depositTokenForOLAS(msg.sender, tokenAmount, token, payout); // ... existing code ... }
Use a try-catch block to handle potential reverts from the external call.
function deposit(uint256 productId, uint256 tokenAmount) external returns (uint256 payout, uint256 maturity, uint256 bondId) { // ... existing code ... try ITreasury(treasury).depositTokenForOLAS(msg.sender, tokenAmount, token, payout) { // Handle success } catch { // Handle failure, e.g., log an event or revert with a custom error message revert("Treasury deposit failed"); } // ... existing code ... }
The contract uses block.timestamp for calculating bond maturity, which can be slightly manipulated by miners.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol
function deposit(uint256 productId, uint256 tokenAmount) external returns (uint256 payout, uint256 maturity, uint256 bondId) { // ... existing code ... maturity = block.timestamp + product.vesting; // ... existing code ... }
While the risk is minimal for long-term vesting periods, it's good practice to document the assumption and consider using an oracle for time-sensitive logic if necessary.
The redeem function transfers tokens without checking the return value.\
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol
function redeem(uint256[] memory bondIds) external returns (uint256 payout) { // ... existing code ... IToken(olas).transfer(msg.sender, payout); }
Use transfer with a require statement to ensure the transfer was successful or use safeTransfer from OpenZeppelin's SafeERC20 library.
The redeem function reads from storage multiple times within a loop, which is costly.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol
function redeem(uint256[] memory bondIds) external returns (uint256 payout) { for (uint256 i = 0; i < bondIds.length; ++i) { uint256 pay = mapUserBonds[bondIds[i]].payout; } }
Cache the storage variable in memory before the loop to save gas.
function redeem(uint256[] memory bondIds) external returns (uint256 payout) { for (uint256 i = 0; i < bondIds.length; ++i) { Bond memory bond = mapUserBonds[bondIds[i]]; uint256 pay = bond.payout; // ... existing code ... } // ... existing code ... }
The redeem function allows a user to redeem multiple bonds in a single transaction. If a user has a large number of bonds, the transaction could run out of gas, effectively locking the user's funds because they are unable to redeem their bonds.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol
function redeem(uint256[] memory bondIds) external returns (uint256 payout) { for (uint256 i = 0; i < bondIds.length; ++i) { // ... existing code ... } // ... existing code ... }
Limit the number of bonds that can be redeemed in a single transaction. Alternatively, implement a paginated redemption mechanism that allows users to redeem their bonds in batches.
The deposit function calculates the payout based on the current price of LP tokens. A malicious actor could observe a pending transaction and execute a transaction with a higher gas price to affect the LP token price before the deposit transaction is confirmed.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol
function deposit(uint256 productId, uint256 tokenAmount) external returns (uint256 payout, uint256 maturity, uint256 bondId) { // ... existing code ... payout = IGenericBondCalculator(bondCalculator).calculatePayoutOLAS(tokenAmount, product.priceLP); // ... existing code ... }
Implement commit-reveal schemes or use an oracle to provide a time-weighted average price (TWAP) to mitigate front-running attacks.
The changeOwner, changeManagers, and changeBondCalculator functions allow the owner to change critical contract addresses. If the owner's private key is compromised, an attacker could redirect funds or disrupt the contract's functionality.
function changeOwner(address newOwner) external { // ... existing code ... owner = newOwner; // ... existing code ... }
Implement a multi-signature mechanism or a time-locked admin function to change critical contract parameters. This would require multiple confirmations before such changes take effect, reducing the risk of a single point of failure.
The contract makes external calls to other contracts, which can be manipulated by malicious actors if those contracts are not trustworthy or have vulnerabilities.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Dispenser.sol
(reward, topUp) = ITokenomics(tokenomics).accountOwnerIncentives(msg.sender, unitTypes, unitIds); ... success = ITreasury(treasury).withdrawToAccount(msg.sender, reward, topUp);
External calls can lead to several issues, such as reentrancy attacks or unexpected failures if the called contract throws an error or runs out of gas.
Use try/catch to handle potential errors from external calls gracefully and ensure that the state of the contract remains consistent.
try ITokenomics(tokenomics).accountOwnerIncentives(msg.sender, unitTypes, unitIds) returns (uint256 _reward, uint256 _topUp) { reward = _reward; topUp = _topUp; } catch { revert ExternalCallFailed("Tokenomics"); } ... try ITreasury(treasury).withdrawToAccount(msg.sender, reward, topUp) { success = true; } catch { revert ExternalCallFailed("Treasury"); }
The contract allows the owner to transfer ownership to a new address. If the owner's private key is compromised, the attacker can transfer ownership to themselves.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Dispenser.sol
function changeOwner(address newOwner) external { if (msg.sender != owner) { revert OwnerOnly(msg.sender, owner); } ... owner = newOwner; emit OwnerUpdated(newOwner); }
Implement a two-step ownership transfer process. This can prevent accidental transfers and provide a recovery window if the owner's key is compromised.
The calculatePayoutOLAS function relies on the state of an external contract (ITokenomics) to fetch the last discount factor (IDF). If the external contract is compromised or behaves unexpectedly, it could affect the integrity of the payout calculation.
amountOLAS = ITokenomics(tokenomics).getLastIDF() * totalTokenValue / 1e36;
Implement a mechanism to verify the integrity of the data returned by the external contract. This could include sanity checks on the values returned, such as ensuring they are within expected ranges, or using a commit-reveal scheme to ensure the data has not been tampered with between updates.
The getCurrentPriceLP function calculates the price based on the reserves of the Uniswap pair. An attacker could temporarily manipulate the reserves by performing a large trade just before the price is fetched, leading to an incorrect price calculation.
priceLP = (reserve1 * 1e18) / totalSupply;
To protect against this, consider using an oracle that provides time-weighted average prices (TWAP) to smooth out temporary price fluctuations. Additionally, implementing slippage protection or maximum price change limits between consecutive calls could help mitigate the impact of price manipulation.
The calculatePayoutOLAS function does not validate the priceLP and tokenAmount inputs, which could lead to unexpected behavior if invalid values are passed.
function calculatePayoutOLAS(uint256 tokenAmount, uint256 priceLP) external view returns (uint256 amountOLAS)
Implement input validation checks to ensure that the values provided are within reasonable and expected ranges. This could include checks against historical data or predefined limits to prevent extreme values from being used in calculations.
The contract interacts with the IUniswapV2Pair interface without checking the success of the call or the validity of the returned data.
(reserve0, reserve1, ) = pair.getReserves();
Mitigation: Ensure that the data returned from external contract calls is valid and that the calls themselves are successful. This could involve checking return values and using try/catch blocks to handle failed calls gracefully.
The checkpoint function uses block.timestamp to determine the end of an epoch. The reliance on block.timestamp can be manipulated by miners to a certain degree and might introduce timing vulnerabilities.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Tokenomics.sol
Consider using block numbers instead of timestamps for epoch calculations, as they are less prone to manipulation. If timestamps must be used, ensure there's a tolerance for the time drift that can occur.
In the trackServiceDonations function, there's a loop that makes external calls to IServiceRegistry.exists. This could lead to a denial of service if the loop grows too large or if the external service is unresponsive.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Tokenomics.sol#L788
Consider validating service existence outside of the loop, or ensure that there's an upper bound on the number of iterations to prevent excessive gas consumption and potential denial of service.
The accountOwnerIncentives function reads from storage multiple times within a loop, which is gas-inefficient.
Cache storage variables in memory before the loop to reduce gas costs.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Tokenomics.sol#L1085
function accountOwnerIncentives(address account, uint256[] memory unitTypes, uint256[] memory unitIds) external returns (uint256 reward, uint256 topUp) { // ... (existing checks and setup) // Cache storage variables in memory uint256 curEpoch = epochCounter; IncentiveBalances memory incentiveBalance; for (uint256 i = 0; i < unitIds.length; ++i) { // Read once from storage incentiveBalance = mapUnitIncentives[unitTypes[i]][unitIds[i]]; // ... (use incentiveBalance instead of mapUnitIncentives for calculations) // Clear storage balances mapUnitIncentives[unitTypes[i]][unitIds[i]].reward = 0; mapUnitIncentives[unitTypes[i]][unitIds[i]].topUp = 0; } // ... (rest of the function) }
The checkpoint function uses block.timestamp to determine the end of an epoch. The reliance on block.timestamp can be manipulated by miners to a certain degree and might introduce timing vulnerabilities.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Tokenomics.sol
Consider using block numbers instead of timestamps for epoch calculations, as they are less prone to manipulation. If timestamps must be used, ensure there's a tolerance for the time drift that can occur.
In the trackServiceDonations function, there's a loop that makes external calls to IServiceRegistry.exists. This could lead to a denial of service if the loop grows too large or if the external service is unresponsive.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Tokenomics.sol#L788
Consider validating service existence outside of the loop, or ensure that there's an upper bound on the number of iterations to prevent excessive gas consumption and potential denial of service.
The accountOwnerIncentives function reads from storage multiple times within a loop, which is gas-inefficient.
Cache storage variables in memory before the loop to reduce gas costs.
https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Tokenomics.sol#L1085
function accountOwnerIncentives(address account, uint256[] memory unitTypes, uint256[] memory unitIds) external returns (uint256 reward, uint256 topUp) { // ... (existing checks and setup) // Cache storage variables in memory uint256 curEpoch = epochCounter; IncentiveBalances memory incentiveBalance; for (uint256 i = 0; i < unitIds.length; ++i) { // Read once from storage incentiveBalance = mapUnitIncentives[unitTypes[i]][unitIds[i]]; // ... (use incentiveBalance instead of mapUnitIncentives for calculations) // Clear storage balances mapUnitIncentives[unitTypes[i]][unitIds[i]].reward = 0; mapUnitIncentives[unitTypes[i]][unitIds[i]].topUp = 0; } // ... (rest of the function) }
The token allowance check happens after initiating the token transfer. This could allow the transfer to fail if allowance is insufficient. Better to validate allowance first.
if (IToken(token).allowance(account, address(this)) < tokenAmount) { revert InsufficientAllowance(...) }
bool success = IToken(token).transferFrom(...) if(!success) { revert TransferFailed(...) }
The rebalanceTreasury function does not validate that the tokenomics address is correctly set. This could allow it to be changed to a malicious contract.
Add check that msg.sender equals the stored tokenomics address variable.
call for ETH transfersUse transfer or send for ETH transfers to prevent reentrancy, or ensure all state changes occur before the call.
(success, ) = to.call{value: tokenAmount}("");
Change:
// Using transfer for safer ETH transfers to.transfer(tokenAmount);
The contract makes external calls to other contracts, which could potentially fail or be manipulated if the called contracts are malicious.
Consider using try/catch for handling external calls that could fail.
ITokenomics(tokenomics).trackServiceDonations(msg.sender, serviceIds, amounts, msg.value);
Change:
try ITokenomics(tokenomics).trackServiceDonations(msg.sender, serviceIds, amounts, msg.value) { // Handle successful call } catch { // Handle failed call }
This function allows the contract owner to drain funds from the ServiceRegistry. However, there is no check that the ServiceRegistry address is set correctly.
Add check that serviceRegistry address matches expected value:
if (serviceRegistry != ITokenomics(tokenomics).serviceRegistry()) { revert Unauthorized(); }
The contract assumes that all token transfers will succeed without explicitly checking the return value. Check the return value of the token transfer function to ensure it was successful.
bool success = IToken(token).transferFrom(account, address(this), tokenAmount); if (!success) { revert TransferFailed(token, account, address(this), tokenAmount); }
Change:
require(IToken(token).transferFrom(account, address(this), tokenAmount), "TransferFrom failed");
The ERC20 standard does not enforce that a transfer must revert on failure. Instead, it should return false. It's important to check the return value to ensure that the transfer was successful.
Add a check to ensure that the contract is not paused before accepting ETH.
receive() external payable { // ... existing logic ... }
After:
receive() external payable whenNotPaused { // ... existing logic ... }
Adding a check for the paused state in the receive function prevents the contract from accepting ETH when it is not operational, which could be a part of the contract's safety mechanism.
The withdraw function assumes that the firstAvailablePositionAccountIndex always points to an account with liquidity. This could lead to a denial of service if no liquidity is available at that index.
address positionAddress = positionAccounts[firstAvailablePositionAccountIndex];
address positionAddress; bool found = false; for (uint32 i = firstAvailablePositionAccountIndex; i < numPositionAccounts; ++i) { if (mapPositionAccountLiquidity[positionAccounts[i]] > 0) { positionAddress = positionAccounts[i]; found = true; break; } } require(found, "No available position with liquidity");
Iterating through the positionAccounts until a position with liquidity is found prevents the function from failing due to an incorrect index. This change ensures that the function does not revert unnecessarily, avoiding a potential denial of service.
This could lead to a loss of funds if the transfer fails silently.
SplToken.transfer(...);
change to:
require( SplToken.transfer(...), "Token transfer failed" );
Checking the return value of the transfer function ensures that the transfer was successful. If the transfer fails, the transaction will revert, preventing any further execution and potential loss of funds.
The original code checks for equality against a single value, which is likely incorrect. The corrected code checks that the tick indices are within the allowed range, ensuring proper validation.
The contract makes external calls without considering the possibility of failure. This could lead to inconsistent state if the external call fails but the contract state has already been updated.
Before:
whirlpool.decreaseLiquidity(...);
change to:
bool success = whirlpool.decreaseLiquidity(...); require(success, "External call to whirlpool failed");
Checking the success of the external call will ensure that the state of the contract only updates if the external call was successful. This prevents inconsistencies in the contract state.
The _getPositionData function checks for specific tick indices, which may not be appropriate for all positions. This could lead to valid positions being rejected.
if (positionData.tickLowerIndex != minTickLowerIndex || positionData.tickUpperIndex != maxTickLowerIndex) { revert("Wrong ticks"); }
Validate the tick indices against a range of acceptable values rather than specific ones. This allows for greater flexibility and prevents legitimate positions from being incorrectly invalidated.
The withdraw function relies on the firstAvailablePositionAccountIndex to point to an account with liquidity. If this index points to an account without liquidity, the function will revert, potentially causing a denial of service for legitimate withdrawals.
address positionAddress = positionAccounts[firstAvailablePositionAccountIndex];
Implement a mechanism to skip over position accounts with zero liquidity. This could involve iterating through the positionAccounts array until an account with sufficient liquidity is found or restructuring how position accounts are tracked to avoid gaps.
The contract does not emit events after sensitive actions such as deposits and withdrawals. This makes it difficult to track these actions off-chain and can be exploited by malicious actors to perform actions without being easily detected.
Emit events after state-changing actions to provide transparency and enable off-chain monitoring. For example, emit a Deposit event after a successful deposit and a Withdrawal event after a successful withdrawal.
The withdraw function does not account for slippage, which could result in users receiving fewer tokens than expected when removing liquidity.
whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, 0, 0);
Include slippage protection by specifying minimum amounts for the tokens to be received when decreasing liquidity. This can be done by calculating the minimum acceptable amounts based on the current pool prices and an acceptable slippage percentage.
The functions pda_mint_to, transfer, and pda_burn make external calls to the Solana token program without checking the success of these calls. If the external call fails, the Solidity contract will not revert, potentially leading to a state of inconsistency or silent failures.
tokenProgramId.call{accounts: metas, seeds: [[seed, bump]]}(instr);
Check Return Values of External Calls To mitigate this issue, the contract should check the return value of the external call and revert if the call was unsuccessful. This ensures that the contract's state remains consistent and that failures are handled appropriately.
(bool success, ) = tokenProgramId.call{accounts: metas, seeds: [[seed, bump]]}(instr); require(success, "External call failed");
By adding the require statement, we ensure that the entire transaction is reverted if the external call does not succeed. This prevents attackers from exploiting failed calls to cause unintended effects in the contract's logic or state.
The functions pda_mint_to and pda_burn are designed to mint and burn tokens, respectively. These are highly sensitive operations that should be restricted to authorized users only. However, the code does not implement any explicit access control mechanisms to restrict who can call these functions.
function pda_mint_to(address mint, address account, address authority, uint64 amount, bytes seed, bytes bump) internal { ... } function pda_burn(address account, address mint, address owner, uint64 amount, bytes seed, bytes bump) internal { ... }
Implement Access Control To mitigate this issue, the contract should implement a robust access control mechanism to ensure that only authorized entities can execute these functions. This can be achieved by using modifiers that check whether the caller has the appropriate role or permissions.
modifier onlyMintAuthority(address authority) { require(msg.sender == authority, "Caller is not the mint authority"); _; }
By adding a modifier onlyMintAuthority, we ensure that only the designated mint authority can mint or burn tokens, which is a critical access control measure to prevent unauthorized token manipulation.
function pda_mint_to(address mint, address account, address authority, uint64 amount, bytes seed, bytes bump) internal onlyMintAuthority(authority) { ... } function pda_burn(address account, address mint, address owner, uint64 amount, bytes seed, bytes bump) internal onlyMintAuthority(owner) { ... }
#0 - c4-pre-sort
2024-01-10T13:46:54Z
alex-ppg marked the issue as sufficient quality report
#1 - c4-judge
2024-01-19T22:05:34Z
dmvt marked the issue as grade-b