Astaria contest - 0x1f8b's results

• Low
  • 1. Allows malleable SECP256K1 signatures
  • 2. Wrong transfer amount
  • 3. Reentrancy pattern
• Non critical
  • 4. Mixing and Outdated compiler
  • 5. Lack of initialize protections
  • 6. Avoid hardcoded values
  • 7. Token compatibility
  • 8. Improve code style

Low

1. Allows malleable SECP256K1 signatures

Here, the ecrecover() method doesn't check the s range.

Homestead (EIP-2) added this limitation, however the precompile remained unaltered. The majority of libraries, including OpenZeppelin, do this check.

Since an order can only be confirmed once and its hash is saved, there doesn't seem to be a serious danger in existing use cases.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7201e6707f6631d9499a569f492870ebdd4133cf/contracts/utils/cryptography/ECDSA.sol#L138-L149

Affected source code:

• ERC20-Cloned.sol:121
• VaultImplementation.sol:246

2. Wrong transfer amount

The amount set by the user when calling transferFrom is not taken into account in the internal method of _execute, so it should be forced to be 1.

  function safeTransferFrom(
    address from, // the from is the offerer
    address to,
    uint256 identifier,
    uint256 amount,
    bytes calldata data //empty from seaport
  ) public {
    //data is empty and useless
    _execute(from, to, identifier, amount);
  }

Affected source code:

• ClearingHouse.sol:173

3. Reentrancy pattern

Although it has not been possible to exploit the reentrancy attack, the logic of the methods named below, make the changes of the validation flags after a method that allows reentrancy, it is convenient to modify the flags before the external calls to contracts.

In the WithdrawProxy.claim method, the s.finalAuctionEnd flag is set after an external contract is called, so if that contract allows a reentrancy attack, the attack vector is possible. It's better to apply the following changes:

  function claim() public {
    WPStorage storage s = _loadSlot();

    if (s.finalAuctionEnd == 0) {
      revert InvalidState(InvalidStates.CANT_CLAIM);
    }
+   s.finalAuctionEnd = 0;
    ...
-   s.finalAuctionEnd = 0;
    emit Claimed(address(this), transferAmount, VAULT(), balance);
  }

Affected source code:

• WithdrawProxy.sol:284

---

Non critical

4. Mixing and Outdated compiler

The pragma version used are:

pragma solidity >=0.5.0;
pragma solidity >=0.7.5;
pragma solidity ^0.8.13;
pragma solidity ^0.8.17;
pragma solidity =0.8.17;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

5. Lack of initialize protections

If you want to use the modifier in the class that implements the base class, you don't need to define the Initializable inheritance in the base class.

The following contracts are updateable, and follow the update pattern, these contracts contain an initialize method to set the contract once, but anyone can call this method, this will spend project fuel if someone tries to initialize it before the project.

It is recommended to check the sender when initializing the contract.

- function __initERC721(string memory _name, string memory _symbol) internal {
+ function __initERC721(string memory _name, string memory _symbol) internal initializer {
    ERC721Storage storage s = _loadERC721Slot();
    s.name = _name;
    s.symbol = _symbol;
  }

Affected source code:

• ERC721.sol:69
• VaultImplementation.sol:190

6. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

Use the selector instead of the raw value:

• ERC721.sol:190-192

7. Token compatibility

The current logic does not allow a token that does not have decimals to be used as a PublicVault asset.

return 10**(ERC20(asset()).decimals() - 1);

Affected source code:

• PublicVault.sol:106

8. Improve code style

Normally the arguments and local variables are written in lower case, however in many contracts it has been found that upper case is used, typically associated with constants, a good code style improves its readability.

Affected source code:

• AstariaRouter.sol:84

Using ' _' at the beginning of public or external methods is not a common practice, and is discouraged, worsening the readability and auditability of the code.

Affected source code:

• AstariaRouter.sol:345
• AstariaRouter.sol:352

• Gas
  • 1. Optimize ClearingHouse._execute
  • 2. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 10 = 130

Gas

1. Optimize ClearingHouse._execute

  function _execute(
    address tokenContract, // collateral token sending the fake nft
    address to, // buyer
    uint256 encodedMetaData, //retrieve token address from the encoded data
    uint256 // space to encode whatever is needed,
  ) internal {
    ...
    uint256 payment = ERC20(paymentToken).balanceOf(address(this));
    ...

+   payment = ERC20(paymentToken).balanceOf(address(this));
+   if (payment > 0) {
-   if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
      ERC20(paymentToken).safeTransfer(
        ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),
-       ERC20(paymentToken).balanceOf(address(this))
+       payment
      );
    }
    ...
  }

Gas diff:

In red the old values, in green the new ones:

- [PASS] testAuctionEnd() (gas: 4061002)
+ [PASS] testAuctionEnd() (gas: 4061001)
- [PASS] testLiquidationAtBoundary() (gas: 8649237)
+ [PASS] testLiquidationAtBoundary() (gas: 8648336)
- [PASS] testLiquidationNftTransfer() (gas: 4215116)
+ [PASS] testLiquidationNftTransfer() (gas: 4214396)
- [PASS] testLiquidationPaymentsOverbid() (gas: 4176825)
+ [PASS] testLiquidationPaymentsOverbid() (gas: 4176104)

- │ execute                                                    ┆ 6716            ┆ 74841 ┆ 56316  ┆ 233288 ┆ 24      │
+ │ execute                                                    ┆ 6716            ┆ 74540 ┆ 55955  ┆ 232567 ┆ 24      │
- │ fulfillAdvancedOrder                                               ┆ 115960          ┆ 150226 ┆ 159400 ┆ 175320 ┆ 3       │
+ │ fulfillAdvancedOrder                                               ┆ 115239          ┆ 149505 ┆ 158679 ┆ 174599 ┆ 3       │

Affected source code:

• ClearingHouse.sol:163

2. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint256 private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
Uint256 private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• PublicVault.sol:174
• PublicVault.sol:179
• PublicVault.sol:380
• PublicVault.sol:405
• PublicVault.sol:565
• PublicVault.sol:583
• PublicVault.sol:606
• PublicVault.sol:624
• WithdrawProxy.sol:237
• WithdrawProxy.sol:313

Total gas saved: 13 * 10 = 130