Astaria contest - gz627's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol#L271-L274 https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L307-L319

Vulnerability details

Impact

When converting assets to withdrawRatio, it projects the ratio from 0 ~ 1 to 0 ~ 1e18; however, when converting withdrawRatio back to assets, it projects the withdrawRatio value from 0 ~ 10**ERC20(asset()).decimals() to 0 ~ 1. The conversion holds only when ERC20(asset()).decimals() == 18.

When using a ERC20 token, which decimals is not 18, as underlying asset of the Vault, the value of transferAmount calculated in WithdrawProxy.claim() is incorrect. This will result in incorrectly transferring assets in WithdrawProxy.claim().

As the result, as long as the asset's decimals is not 18, it's easy for strategists/borrowers/LPs to exploit this vulnerability depending on the value of its decimals larger or less than 18.

POC

The reasons are:

1. withdrawRatio is calculated in PublicVault.processEpoch() from L314 to L316. We can see that the value of withdrawRatio is projected to 1e18 when it's 100%. L318 set the calculated withdrawRatio to currentWithdrawProxy.

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol

        // reset liquidationWithdrawRatio to prepare for re calcualtion
308:    s.liquidationWithdrawRatio = 0;

        // check if there are LPs withdrawing this epoch 
        if ((address(currentWithdrawProxy) != address(0))) {
          uint256 proxySupply = currentWithdrawProxy.totalSupply();

//@audit The below formula projects the ratio from `0 ~ 1` to `0 ~ 1e18`
314:      s.liquidationWithdrawRatio = proxySupply
315:        .mulDivDown(1e18, totalSupply())
316:        .safeCastTo88();

318:      currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
          uint256 expected = currentWithdrawProxy.getExpected();

2. When the withdrawRatio is converted back to the value of asset in WithdrawProxy.claim(), it uses 10**ERC20(asset()).decimals() to calculate the actual amount of asset on L271-273. This will be correct only when 10**ERC20(asset()).decimals() == 1e18. However, not all ERC20 decimals are 18.

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol

271:      transferAmount = uint256(s.withdrawRatio).mulDivDown(
            balance,
273:        10**ERC20(asset()).decimals()
          );

Tools Used

Manual audit

Recommended Mitigation Steps

Change L273 of WithdrawProxy.claim() to 1e18 will solve the issue:

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol

271:      transferAmount = uint256(s.withdrawRatio).mulDivDown(
            balance,
273:        1e18
          );

QA report

Non-Critical Issues

[NC-1] Unused stack variables should be removed

Instances (5):

There are some variables previously used but now not used. These variables should be removed for gas efficiency and maintenance.

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol

138:    address tokenContract = underlying.tokenContract;  //@audit To remove this line

139:    uint256 tokenId = underlying.tokenId;  //@audit To remove this line

342:     address tokenContract = underlying.tokenContract;  //@audit To remove this line

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol

632:    uint256 end = stack[position].end;  //@audit To remove this line

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol

262:    uint256 assets = totalAssets();  //@audit To remove this line

---

[NC-2] Unused function parameters can be commented to improve readability

Instances (23):

In some functions, there are unused function parameters for some reasons such as satisfying interfaces etc. These unused function parameters can be commented for the benefits of:

1. Improved readability;
2. Reduced warnings from linting and compiling.

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol

82:   function balanceOf(address account, uint256 id)  //@audit `account` and `id` are not used in function body
        external
        view
        returns (uint256)
      {
        return type(uint256).max;
      }

The optimized code:

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol

82:   function balanceOf(address /* account */, uint256 /* id */)
        external
        view
        returns (uint256)
      {
        return type(uint256).max;
      }

Other instances:

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol

90:  function balanceOfBatch(address[] calldata accounts, uint256[] calldata ids)  //@audit To comment `ids`

104:  function setApprovalForAll(address operator, bool approved) external {}  //@audit To comment `operator`, `approved`

106:  function isApprovedForAll(address account, address operator)  //@audit To comment `account`, `operator`

115:       address tokenContract,   //@audit To comment `tokenContract`,    // collateral token sending the fake nft

116:       address to,   //@audit To comment `to`,    // buyer

174:        bytes calldata data   //@audit To comment `data`,    //empty from seaport

189:        address operator_,  //@audit To comment `operator_`
190:        address from_,  //@audit To comment `from_`
191:        uint256 tokenId_,  //@audit To comment `tokenId_`
192:        bytes calldata data_  //@audit To comment `data_`
        

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol

159:    address caller,  //@audit To comment `caller`
160:    address offerer,  //@audit To comment `offerer`

173:    address caller,  //@audit To comment `caller`
    
175:    bytes32[] calldata priorOrderHashes,  //@audit To comment `priorOrderHashes`
176:    CriteriaResolver[] calldata criteriaResolvers  //@audit To comment `priorOrderHashes`

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol

775:    function _getRemainingInterest(LienStorage storage s, Stack memory stack)  //@audit To comment `s`

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol

413:    function _beforeCommitToLien(IAstariaRouter.Commitment calldata params)  //@audit To comment `params`

575:    function afterDeposit(uint256 assets, uint256 shares)   //@audit To comment `shares`

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol

143:    function deposit(uint256 assets, address receiver)   //@audit To comment `assets`, `receiver`

---

[NC-3] Stack variables shawdowed state variables

Instances (11):

There are some stack variables shawdowed state variables. It is suggested to change the stack variables for the benefits of:

• improved readability;
• better code maintenance experience;
• reduced linting warning messages.

Link to original code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L110-L124

110:  /**
       * @notice Signal a withdrawal of funds (redeeming for underlying asset) in the next epoch.
       * @param shares The number of VaultToken shares to redeem.
       * @param receiver The receiver of the WithdrawTokens (and eventual underlying asset)
       * @param owner The owner of the VaultTokens.
       * @return assets The amount of the underlying asset redeemed.
       */
117:  function redeem(
        uint256 shares,
        address receiver,
120:    address owner    //@audit `owner` shawdowed state variable
      ) public virtual override(ERC4626Cloned) returns (uint256 assets) {
        VaultData storage s = _loadStorageSlot();
123:    assets = _redeemFutureEpoch(s, shares, receiver, owner, s.currentEpoch);  //@audit `owner` shawdowed state variable
      }

The optimized code: Change owner to _owner

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L110-L124

110:  /**
       * @notice Signal a withdrawal of funds (redeeming for underlying asset) in the next epoch.
       * @param shares The number of VaultToken shares to redeem.
       * @param receiver The receiver of the WithdrawTokens (and eventual underlying asset)
       * @param _owner The owner of the VaultTokens.  //@audit Change `owner` to `_owner`
       * @return assets The amount of the underlying asset redeemed.
       */
117:  function redeem(
        uint256 shares,
        address receiver,
120:    address _owner      //@audit Change `owner` to `_owner`
      ) public virtual override(ERC4626Cloned) returns (uint256 assets) {
        VaultData storage s = _loadStorageSlot();
123:    assets = _redeemFutureEpoch(s, shares, receiver, _owner, s.currentEpoch);    //@audit Change `owner` to `_owner`
      }

Other instances:

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L110-L124

129:  address owner  //@audit Change `owner` to `_owner`

135:  _redeemFutureEpoch(s, shares, receiver, owner, s.currentEpoch);  //@audit Change `owner` to `_owner`

141:  address owner,  //@audit Change `owner` to `_owner`

145:     _redeemFutureEpoch(_loadStorageSlot(), shares, receiver, owner, epoch);  //@audit Change `owner` to `_owner`

152:  address owner,  //@audit Change `owner` to `_owner`

159:     if (msg.sender != owner) {  //@audit Change `owner` to `_owner`

160:         uint256 allowed = es.allowance[owner][msg.sender]; // Saves gas for limited approvals.  //@audit Change `owner` to `_owner`

163:         es.allowance[owner][msg.sender] = allowed - shares;  //@audit Change `owner` to `_owner`

174:      es.balanceOf[owner] -= shares;  //@audit Change `owner` to `_owner`

182:      emit Transfer(owner, address(this), shares);  //@audit Change `owner` to `_owner`

---

[NC-4] Remove redundant code

Instances (1):

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol

331:  function releaseToAddress(uint256 collateralId, address releaseTo)
        public
        releaseCheck(collateralId)
334:    onlyOwner(collateralId)
      {
        CollateralStorage storage s = _loadCollateralSlot();

338:    if (msg.sender != ownerOf(collateralId)) {  //@audit L338-340 has the same functionality as L334 and can be removed
          revert InvalidSender();
340:    }
        Asset memory underlying = s.idToUnderlying[collateralId];
        address tokenContract = underlying.tokenContract;
        _burn(collateralId);
        delete s.idToUnderlying[collateralId];
        _releaseToAddress(s, underlying, collateralId, releaseTo);
      }

In the above function L334 onlyOwner(collateralId) has the same functionality as L338-340. Thus, L338-340 can be removed.

---

---

Low risk issues

[L-1] Optimize Vault.deposit

Link to code

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L59-L68

59:   function deposit(uint256 amount, address receiver)
        public
        virtual
        returns (uint256)
      {
64:     VIData storage s = _loadVISlot();
65:     require(s.allowList[msg.sender] && receiver == owner());
        ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
        return amount;
68:   }

The Dev Team explains that, in the above function deposit(uint256 amount, address receiver), the receiver parameter is used to match erc4626 interface only, and private vault only allows the owner to deposit. So the function can be optimized as in the below:

1. Replace require(s.allowList[msg.sender] && receiver == owner()); with if (msg.sender != owner()) revert OnlyOwnerDepositable();. This optimization offers: (1) simplifying coding logic and still satisfying the function purpose; (2) saving gas by removing un-necessary state variable and introducing custom error.
2. Comment function parameter receiver: This improves readability and eliminates linting warning.
3. Remove Line 64: This saves significant gas.

The optimized code:

File: https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L59-L68

     error OnlyOwnerDepositable();  //@audit Introduce custom error for gas efficiency

59:   function deposit(uint256 amount, address /* receiver */)  //@audit Comment `receiver`
        public
        virtual
        returns (uint256)
      {
64:     // VIData storage s = _loadVISlot();  //@audit This line can be removed
65:     if (msg.sender != owner()) revert OnlyOwnerDepositable();  //@audit Use costom error to save gas
        ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
        return amount;
68:   }