- Scope: https://github.com/ebtc-protocol/ebtc/tree/1a45e716c833867efdf5b8cb470a367196616480/packages/contracts/contracts
- Timeline: Jan 8 - 25, 2024
- Report Version: Feb 5, 2024
In certain cases, SortedCdps._cdpOfOwnerByIndex, SortedCdps._cdpCountOf, and SortedCdps._getCdpsOf functions will return invalid values. This is because these functions accept invalid startNodeIds.
The core issue lies in the checks getOwnerAddress(_currentCdpId) == owner, which permits users to submit non-existing cdpIds that resolve to specific owners. Because a cdpId includes its owner in its first 20 bytes, one can supply a cdpId such as 0x01, which will return address(0) as its owner.
function getOwnerAddress(bytes32 cdpId) public pure override returns (address) {
uint256 _tmp = uint256(cdpId) >> ADDRESS_SHIFT;
return address(uint160(_tmp));
}Examining the _cdpCountOf function below, we can see that any address' _ownedCount can be be made to return 1.
For example, function getCdpCountOf(owner = address(0), startNodeId = 0x01, maxNodes = 0 will incorrectly return 1. Below, we can see that _ownedCount will be incremented by 1 in the first while loop iteration, because getOwnerAddress(_currentCdpId) == owner will return true for the aforementioned input arguments.
function _cdpCountOf(
address owner,
bytes32 startNodeId,
uint maxNodes
) internal view returns (uint256, bytes32) {
// walk the list, until we get to the count
// start at the given node or from the tail of list
bytes32 _currentCdpId = (startNodeId == dummyId ? data.tail : startNodeId);
uint _ownedCount = 0;
uint i = 0;
while (_currentCdpId != dummyId) {
// if the current CDP is owned by specified owner
if (getOwnerAddress(_currentCdpId) == owner) {
_ownedCount = _ownedCount + 1;
}
++i;
// move to the next CDP in the list
_currentCdpId = data.nodes[_currentCdpId].prevId;
// cut the run if we exceed expected iterations through the loop
if (maxNodes > 0 && i >= maxNodes) {
break;
}
}
return (_ownedCount, _currentCdpId);
}One can also submit a non-existent startNodeId that resolves to their address as the owner. For example, I could submit owner = 0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045 (vitalik.eth) and startNodeId = 0x000000000000000000000000d8da6bf26964af9d7eed9e03e53415d37aa96045, which will return 1, regardless of how much cdp is owned by vitalik.eth.
Note that such calls to getCdpCountOf() will always return 1, because data.nodes[_currentCdpId].prevId returns dummyId and prevents a second iteration.
getAllCdpsOf() and cdpOfOwnerByIdx() functions can also be tricked in the same fashion.
Although these functions are not internally used, an external contract depending on them can potentially be exploited.
Consider returning early with null values when startNodeId does not exist in the sorted list.
Cdps stored in CdpManagerStorage have an associated arrayIndex property that points to their position in the CdpIds array. When closing a CDP, all properties of the CDP except its arrayIndex is cleaned up. There are multiple paths to close a CDP, but to give an example, a path would be CdpManager.closeCdp ⟶ CdpManagerStorage._closeCdp ⟶ ._closeCdpWithoutRemovingSortedCdps ⟶ ._removeCdp.
Moreover, within CdpManagerStorage._removeCdp, the last CDP in the CdpIds array is moved to the position of the removed CDP. This means that the arrayIndex of both of these CDPs point to the same position (unless the removed CDP was already at the last position in the array).
The lingering arrayIndex value of the removed CDP is exposed to public through Cdps(id).arrayIndex. This would lead users to the wrong CDP, if they for some reason do something like Cdps(CdpIds(Cdps(id).arrayIndex)).
Consider deleting CDP's arrayIndex during removal.
Info: Immutable variables in CdpManagerStorage must be set the same in LiquidiationLibrary and CdpManager
CdpManager delegates liquidation functionality to LiquidationLibrary through a delegatecall. All CdpManager contract state resides in CdpManagerStorage, hence LiquidationLibrary can write to the correct storage slots. One caveat of this design that the immutable variables in CdpManagerStorage should be set the same both in CdpManager and LiquidationLibrary, because immutable variables are part of the code and will use the values in the implementation instead of the contract making the delegatecall. The correctness of the immutable variables can be ensured through the deployment script, however immutable variables through global variables such as block.timestamp, the implementation and proxy values can differ. This is the case for deploymentStartTime, which is initialized to the block.timestamp. However, this variable is not used in LiquidationLibrary, so there is no risk involved in this version.
For EBTCToken.transfer() and transferFrom(), recipient is checked to be not itself, zero address, borrower operations, or CDP manager.
function _requireValidRecipient(address _recipient) internal view {
require(
_recipient != address(0) && _recipient != address(this),
"EBTC: Cannot transfer tokens directly to the EBTC token contract or the zero address"
);
require(
_recipient != cdpManagerAddress && _recipient != borrowerOperationsAddress,
"EBTC: Cannot transfer tokens directly to the CdpManager or BorrowerOps"
);
}The same check is not included in mint(). Although minting to restricted addresses through CdpManager or BorrowerOperations is not possible, a new authorized minter address could make it possible minting EBTC to the restricted address.
Info: Restricted ActivePool.sweepToken() function has a comment specifying that the function can be unrestricted
requiresAuth modifier can be removed as the comment suggests. Consider either removing the comment or the modifier.
/// @dev Function to move unintended dust that are not protected
/// @notice moves given amount of given token (collateral is NOT allowed)
/// @notice because recipient are fixed, this function is safe to be called by anyone
/// @param token The token address to be swept
/// @param amount The token amount to be swept
function sweepToken(address token, uint256 amount) public nonReentrant requiresAuth {
ICdpManagerData(cdpManagerAddress).syncGlobalAccountingAndGracePeriod(); // Accrue State First
require(token != address(collateral), "ActivePool: Cannot Sweep Collateral");
uint256 balance = IERC20(token).balanceOf(address(this));
require(amount <= balance, "ActivePool: Attempt to sweep more than balance");
address cachedFeeRecipientAddress = feeRecipientAddress; // Saves an SLOAD
IERC20(token).safeTransfer(cachedFeeRecipientAddress, amount);
emit SweepTokenSuccess(token, amount, cachedFeeRecipientAddress);
}BorrowerOperations.flashLoan() function will transfer EBTC to the feeRecipient. However EBTC has a restriction on which addresses can receive EBTC token, such as the BorrowerOperations address itself. Setting the feeRecipient to such an address will prevent any flash loans.
Consider adding a check to setFeeRecipientAddress() to ensure feeRecipient is a valid EBTC recipient address.
CdpManagerStorage is not intended to be deployed independently, hence it should be marked as an abstract contract.
In many places of the code there are comments that start with /// @audit or TODO. Consider removing these comments and incorporating them as NatSpec or inline comments where necessary.
contracts/CdpManagerStorage.sol:766: /// @audit-ok We don't take the fee if we had a negative rebase
contracts/Dependencies/TellorCaller.sol:22: // TODO: Use new Tellor query ID for stETH/BTC when available
contracts/Dependencies/EbtcBase.sol:143: /// @audit is _icr <= _tcr more dangerous for overal system safety?
contracts/Dependencies/EbtcBase.sol:144: /// @audit Should we use _icr < CCR to allow any risky CDP being liquidated?
contracts/LeverageMacroBase.sol:501: /// @audit Check and add more if you think it's better
contracts/SortedCdps.sol:29: * The list is a modification of the following audited SortedDoublyLinkedList:
contracts/SyncedLiquidationSequencer.sol:74: uint256 _icr = cdpManager.getSyncedICR(_cdpId, _price); /// @audit This is view ICR and not real ICR
contracts/TestContracts/AccruableCdpManager.sol:31: /// @audit Based on the idea that Foundry and Echidna will not fork mainnet
contracts/TestContracts/AccruableCdpManager.sol:32: // require(block.chainid != 1, "No prod!!!!"); /// @audit CANNOT SET, PLS HAAAALP
contracts/TestContracts/invariants/BeforeAfter.sol:93: vars.isRecoveryModeBefore = crLens.quoteCheckRecoveryMode() == 1; /// @audit crLens
contracts/TestContracts/invariants/BeforeAfter.sol:147: vars.isRecoveryModeAfter = cdpManager.checkRecoveryMode(vars.priceAfter); /// @audit This is fine as is because after the system is synched
contracts/TestContracts/invariants/echidna/EchidnaPriceFeedTester.sol:239: // TODO: this is hard to test, as we may have false positives due to the random nature of the tests
contracts/TestContracts/invariants/Properties.sol:74: // @audit We have an instance of getting above 1e18 in rounding error, see `testBrokenInvariantFive`
contracts/TestContracts/invariants/Properties.sol:107: /** TODO: See EchidnaToFoundry._getValue */
contracts/TestContracts/invariants/Properties.sol:140: // TODO remove tolerance once proper fix has been applied
contracts/TestContracts/invariants/Properties.sol:214: /// @audit Precision Threshold to flag very scary scenarios
contracts/TestContracts/invariants/Properties.sol:237: // TODO how to calculate "the dollar value of eBTC"?
contracts/TestContracts/invariants/Properties.sol:238: // TODO how do we take into account underlying/shares into this calculation?
contracts/TestContracts/invariants/Properties.sol:341: /// @audit 1e8 precision
contracts/TestContracts/invariants/TargetContractSetup.sol:86: /// @audit NOTE: This is the TEST contract!!!
contracts/TestContracts/invariants/TargetFunctions.sol:65: ans[i++] = Cdp({id: currentCdp, icr: cdpManager.getSyncedICR(currentCdp, _price)}); /// @audit NOTE: Synced to ensure it's realistic
contracts/TestContracts/invariants/TargetFunctions.sol:542: // TODO: CHECK THIS
contracts/TestContracts/invariants/TargetFunctions.sol:983: // TODO verify the assumption below, maybe there's a more sensible (or Governance-defined/hardcoded) limit for the maximum amount of minted eBTC at a single operation
contracts/ActivePool.sol:209: _setValue(uint128(cachedSystemDebt)); // @audit update TWAP global spot value and accumulator variable along with a timestamp
contracts/ActivePool.sol:210: update(); // @audit update TWAP Observer accumulator and weighted average
contracts/ActivePool.sol:225: _setValue(uint128(cachedSystemDebt)); // @audit update TWAP global spot value and accumulator variable along with a timestamp
contracts/ActivePool.sol:226: update(); // @audit update TWAP Observer accumulator and weighted average
contracts/CdpManager.sol:80: // Cdp ICR | Liquidation Behavior (TODO gas compensation?)
contracts/CdpManager.sol:144: Cdps[_redeemColFromCdp.cdpId].debt /// @audit Redeem everything
contracts/CdpManager.sol:351: totals.twapSystemDebtAtStart = EbtcMath._min(activePool.observe(), systemDebtAtStart); // @audit Return the smaller value of the two, bias towards a larger redemption scaling fee
contracts/CdpManager.sol:398: _syncAccounting(_cId); /// @audit This happens even if the re-insertion doesn't
contracts/CdpManager.sol:535: /// @audit Opening can cause invalid reordering of Cdps due to changing values without reInserting into sortedCdps
contracts/CdpManager.sol:939: cdpStEthFeePerUnitIndex[_cdpId] = systemStEthFeePerUnitIndex; /// @audit We critically assume global accounting is synced here
contracts/ChainlinkCaller.sol:86: // TODO: Maybe add max and min checks as well
contracts/LiquidationLibrary.sol:74: uint256 _ICR = getCachedICR(_cdpId, _price); // @audit syncAccounting already called, guarenteed to be synced
contracts/LiquidationLibrary.sol:572: /// @audit MUST be like so, else we have debt redistribution, which we assume cannot happen in partial
contracts/PriceFeed.sol:375: /// @audit This should never be used, but we added it for the Certora Prover
There are a lot of compilation warnings.
Warning (4591): There are more than 256 warnings. Ignoring the rest.
At the absolute minimum, issues pointed out for the deployment code (i.e.: excluding the testing suite) should be fixed. Ideally, all issues in all contracts should be fixed.
Most of the issues are concerning shadowed declarations, function mutability modifier, and unused variables. These all have simple fixes and they should be dealt with.