Platform: Code4rena
Start Date: 05/01/2023
Pot Size: $90,500 USDC
Total HM: 55
Participants: 103
Period: 14 days
Judge: Picodes
Total Solo HM: 18
Id: 202
League: ETH
Rank: 72/103
Findings: 1
Award: $51.32
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: ladboy233
Also found by: 0x1f8b, 0xAgro, 0xSmartContract, 0xbepresent, 0xkato, Aymen0909, CodingNameKiki, Cryptor, Deekshith99, Deivitto, HE1M, IllIllI, Kaysoft, Koolex, PaludoX0, Qeew, RaymondFam, Rolezn, Sathish9098, Tointer, a12jmx, arialblack14, ast3ros, ayeslick, bin2chen, btk, caventa, ch0bu, chaduke, chrisdior4, delfin454000, descharre, evan, fatherOfBlocks, georgits, gz627, jasonxiale, joestakey, kaden, lukris02, nicobevi, nogo, oberon, oyc_109, pfapostol, rbserver, sakshamguruji, seeu, shark, simon135, slvDev, synackrst, tnevler, whilom, zaskoh
51.3151 USDC - $51.32
| Issue | Description | Instances |
|---|---|---|
| 1 | Outdated Open Zeppelin versions | 5 |
| Total | 5 |
| Issue | Description | Instances |
|---|---|---|
| 1 | Constant value definitions that include call to keccak256 should use immutable | 13 |
| 2 | Remove references to pragma experimental ABIEncoderV2 | 2 |
| 3 | Some comments could be converted to Natspec statements | 3 + multiple |
| 4 | Long single-line comments | 32 |
| 5 | Incorrect comment syntax | 1 |
| 6 | Update sensitive term | 1 |
| 7 | Typos | 6 |
| Total | 58 + multiple |
| No. | Description |
|---|---|
| 1 | Outdated Open Zeppelin versions |
The versions used have known vulnerabilties, all of which have been patched by version 4.7.3. See Security Advisories. |
// OpenZeppelin Contracts v4.4.1 (utils/introspection/IERC165.sol)
// OpenZeppelin Contracts (last updated v4.6.0) (token/ERC721/IERC721Receiver.sol)
// OpenZeppelin Contracts (last updated v4.5.0) (token/ERC1155/IERC1155Receiver.sol)
// OpenZeppelin Contracts (last updated v4.6.0) (token/ERC20/IERC20.sol)
// OpenZeppelin Contracts v4.4.1 (token/ERC1155/IERC1155.sol)
| No. | Description |
|---|---|
| 1 | Constant value definitions that include call to keccak256 should use immutable |
Constant value definitions including a call to keccak256 should use immutable instead of constant |
uint256 private constant CLEARING_HOUSE_STORAGE_SLOT = uint256(keccak256("xyz.astaria.ClearingHouse.storage.location")) - 1;
uint256 private constant WITHDRAW_PROXY_SLOT = uint256(keccak256("xyz.astaria.WithdrawProxy.storage.location")) - 1;
uint256 private constant COLLATERAL_TOKEN_SLOT = uint256(keccak256("xyz.astaria.CollateralToken.storage.location")) - 1;
uint256 private constant PUBLIC_VAULT_SLOT = uint256(keccak256("xyz.astaria.PublicVault.storage.location")) - 1;
uint256 private constant PUBLIC_VAULT_SLOT = uint256(keccak256("xyz.astaria.PublicVault.storage.location")) - 1;
uint256 private constant LIEN_SLOT = uint256(keccak256("xyz.astaria.LienToken.storage.location")) - 1;
uint256 constant ERC20_SLOT = uint256(keccak256("xyz.astaria.ERC20.storage.location")) - 1;
bytes32 private constant PERMIT_TYPEHASH = keccak256( "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" );
uint256 private constant ERC721_SLOT = uint256(keccak256("xyz.astaria.ERC721.storage.location")) - 1;
VaultImplementation.sol: L44-45
bytes32 public constant STRATEGY_TYPEHASH = keccak256("StrategyDetails(uint256 nonce,uint256 deadline,bytes32 root)");
VaultImplementation.sol: L47-50
bytes32 constant EIP_DOMAIN = keccak256( "EIP712Domain(string version,uint256 chainId,address verifyingContract)" );
bytes32 constant VERSION = keccak256("0");
VaultImplementation.sol: L57-58
uint256 private constant VI_SLOT = uint256(keccak256("xyz.astaria.VaultImplementation.storage.location")) - 1;
| No. | Description |
|---|---|
| 2 | Remove references to pragma experimental ABIEncoderV2 |
Remove references to pragma experimental ABIEncoderV2, which has been depreciated. As of Solidity v0.8.0, ABI coder v2 is it is activated by default by the compiler. |
pragma experimental ABIEncoderV2;
pragma experimental ABIEncoderV2;
| No. | Description |
|---|---|
| 3 | Some comments could be converted to Natspec statements |
Some comment lines that explain the purpose of functions could appropriately be converted into Natspec statements, in particular, @notice or @dev. Below are just a few examples: |
* Returns the end timestamp of the last auction tracked by this WithdrawProxy. After this timestamp has passed, claim() can be called.
Suggestion:
* @notice Returns the end timestamp of the last auction tracked by this WithdrawProxy. * After this timestamp has passed, claim() can be called.
Note that I have wrapped the @notice statement above and @dev statement below since they exceed 120 characters
* The rate for the LienToken is subtracted from the total slope of the PublicVault, and recalculated in afterPayment().
Suggestion:
* @dev The rate for the LienToken is subtracted from the total slope of * the PublicVault, and recalculated in afterPayment().
* Calculates the debt accrued by all liens against a CollateralToken, assuming no payments are made until the provided timestamp.
Suggestion:
* @notice Calculates the debt accrued by all liens against a CollateralToken, * assuming no payments are made until the provided timestamp.
| No. | Description |
|---|---|
| 4 | Long single-line comments |
| In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if longer comments (up to 120 characters) are becoming acceptable and a scroll bar is provided, very long comments can interfere with readability. | |
| Many of the long comments in Astoria do wrap. However, below are instances of extra-long comments (over 120 characters) whose readability could be improved by wrapping, as shown. | |
| Note that a small indentation is used in each continuation line (which flags for the reader that the comment is ongoing), along with a period at the close (to signal the end of the comment). In addition, if a line containing both code and a comment is longer than 120 characters, it makes sense to put the comment in the line above. |
* @notice This contract handles the creation, payments, buyouts, and liquidations of tokenized NFT-collateralized debt (liens). Vaults which originate loans against supported collateral are issued a LienToken representing the right to loan repayments and auctioned funds on liquidation.
Suggestion:
* @notice This contract handles the creation, payments, buyouts, and liquidations of tokenized * NFT-collateralized debt (liens). Vaults which originate loans against supported collateral are * issued a LienToken representing the right to loan repayments and auctioned funds on liquidation.
* @param params The Commitment information containing the loan parameters and the merkle proof for the strategy supporting the requested loan.
Suggestion:
* @param params The Commitment information containing the loan parameters and * the merkle proof for the strategy supporting the requested loan.
* @return withdrawProxyIfNearBoundary The address of the WithdrawProxy to set the payee to if the liquidation is triggered near an epoch boundary.
Suggestion:
* @return withdrawProxyIfNearBoundary The address of the WithdrawProxy to set * the payee to if the liquidation is triggered near an epoch boundary.
Similarly for the following extra-long lines:
| No. | Description |
|---|---|
| 5 | Incorrect comment syntax |
// // slope does not need to be updated if paying off the rest, since we neutralize slope in beforePayment()
Change to:
// slope does not need to be updated if paying off the rest, since we neutralize slope in beforePayment()
| No. | Description |
|---|---|
| 6 | Update sensitive term |
Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice and this practice appears to have been adopted in Astaria. In fact, allowlist and its variants are used throughout instead of whitelist, with the exception of the comment below, which appears to be an inadvertant leftover. |
* @param allowListEnabled Whether or not the Vault has an LP whitelist.
Suggestion: Change whitelist to allowList
| No. | Description |
|---|---|
| 7 | Typos |
//invalid action private vautls can only be the owner or strategist
Change vautls to vaults
// reset liquidationWithdrawRatio to prepare for re calcualtion
Change re calcualtion to recalculation
// prevent transfer of more assets then are available
Change then to than
// update the public vault state and get the liquidation accountant back if any
Change accountant to amount, if that was what was intended
The same typo (acheive) occurs in both lines below:
IV3PositionManager.sol: L122-123
/// @return amount0 The amount of token0 to acheive resulting liquidity /// @return amount1 The amount of token1 to acheive resulting liquidity
Change acheive to achieve in both cases
#0 - c4-judge
2023-01-22T17:04:58Z
Picodes marked the issue as grade-b