Astaria contest - PaludoX0's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L595 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L819

Vulnerability details

Impact

Dilutive fees (on lien repayments) for strategists in VaultTokens are calculated using a wrong parmeter.

According to NatSpec PublicVault.sol#L595 parameter amount of function PublicVault:_handleStrategistInterestReward represent The amount that was paid., intended as the amount that is going to be paid during payment in process.

Function beforePayment calls at PublicVault.sol#L526 function _handleStrategistInterestReward passing params.amount as argument. Function _payment calls at src/LienToken.sol#L819 function beforePayment passing amount: stack.point.amount, stack.point.amount corresponds to the total remaining amount of debt to be paid before subtracting the amount of payment in process, not the amount that was paid.

In _handleStrategistInterestReward fee shares are calculated by multiplying FEE_VAULT times the smaller between amount or interestOwing (which is equal to owed - stack.point.amount).

The impact is that if interestOwing is grater than amount paid, the fee passed to the strategist are greater.

Proof of Concept

stack.point.amount = 300 owed = 400 amountRepaid = 50 The fees will be calculated multiplying (400-300)xFEE_VAULT, instead of = 50xFEE_VAULT

Tools Used

Forge foundry

Recommended Mitigation Steps

In src/LienToken.sol#L819 change amount: stack.point.amount to amount: amount

LOW

The event AllowListUpdated is emitted but the allowList is not updated

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L215 At above line the event AllowListUpdated is triggered, but the function modifyAllowList(delegate_, true) is not called and neither required.

Compiler warnings

Compiler warnings should be not ignored, can lead to undesired effects. I suggest at least to fix the following one that could cause issues src/AstariaRouter.sol:359:25: function fileGuardian(File[] calldata file) This declaration shadows an existing declaration: src/AstariaRouter.sol:273:3: function file(File calldata incoming) public requiresAuth

Unused function parameters

Unused function parameter. Remove or comment out the variable name to silence the warnings. src/ClearingHouse.sol:82 src/ClearingHouse.sol:90 src/ClearingHouse.sol:106 src/ClearingHouse.sol:115 and 116 src/ClearingHouse.sol:174 src/ClearingHouse.sol:189, 190 and 191 src/LienToken.sol:775 src/CollateralToken.sol:159 and 160 src/CollateralToken.sol:173 175 and 176 src/WithdrawProxy.sol:143 src/WithdrawProxy.sol:147 src/PublicVault.sol:413 src/PublicVault.sol:575 src/security/V3SecurityHook.sol:25

Duplicated require()/revert() checks should be refactored to a modifier or function

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L78 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.soll#L96 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L105 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L114 require(msg.sender == owner()); can be replaced by a modifier I tested that it doesn't save gas but it keeps code a little bit more neat

Wrong returns parameter

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L717 timeToSecondEpochEnd in returns statement is useless

 function _timeToSecondEndIfPublic()  internal view override returns (uint256 timeToSecondEpochEnd)
 { return timeToEpochEnd() + EPOCH_LENGTH();}

Unused struct declaration

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/interfaces/ICollateralToken.sol#L68 ListUnderlyingForSaleParams struct seem to be unused in the contract, moreover maxDuration type seems to be a typo

struct ListUnderlyingForSaleParams {
   ILienToken.Stack[] stack;
   uint256 listPrice;
   uint56 maxDuration; }

##INFORMATIONAL

Wrong comments

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L37 - 44 should be 41 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L47 - 64 should be 61 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L55 - 116 should be 125

Suggest to change function from newVault() to newPrivateVault()

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L521 This change would make it easier for a newcomer to understand differences between publicVault and privateVault entry point.

NatSpec parameters are wrong

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L166 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/interfaces/ILienToken.sol#L94, L138 and L145 In the above lines NatSpec is wrong due to copy&paste error

Repeated Storage pointer declaration and initialize

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L608 There are through the contract several repeated declaration and initialization of the same storage pointer in the function and in modifier called by the same function. I report as example functions makePayment which calls validateSTack modifier. Both function declare and initialize storage pointer LienStorage storage s = _loadLienStorageSlot(); By changing function makePayment as follows, it saves 1486 gas when calling testBasicPublicVaultLoan()

function makePayment(
uint256 collateralId,
Stack[] calldata stack,
uint8 position,
uint256 amount)
external
//validateStack(collateralId, stack)
returns (Stack[] memory newStack) {
LienStorage storage s = _loadLienStorageSlot();
bytes32 stateHash = s.collateralStateHash[collateralId];
if (stateHash == bytes32(0) && stack.length != 0) {
  revert InvalidState(InvalidStates.EMPTY_STATE);  }
if (stateHash != bytes32(0) && keccak256(abi.encode(stack)) != stateHash) {
  revert InvalidState(InvalidStates.INVALID_HASH);
}
(newStack, ) = _payment(s, stack, position, amount, msg.sender);
_updateCollateralStateHash(s, collateralId, newStack); }

Validate commitment at earlier stage

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L511 Function commitToLiens(IAstariaRouter.Commitment[] memory commitments) is called for depositing collateral and requesting loans for multiple NFTs in one go. Input parameter is an array of IAstariaRouter.Commitment[]. The input is validated by function _validateCommitment at end of a long function calls chain: IAstariaRouter.commitToLiens() -> _executeCommitment() -> IVaultImplementation.commitToLien ->_requestLienAndIssuePayout -> _validateCommitment The validation of commitment can be pushed up to IAstariaRouter.commitToLiens since _validateCommitment parameters are already known at this stage: IAstariaRouter.Commitment calldata params, address receiver This would save a lot of gas in case validation of commitment fails.

Decalaration of currentWithdrawProxy in the same function

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L366 AND L397 address currentWithdrawProxy = s.epochData[s.currentEpoch - 1].withdrawProxy; is declared twice in the same function. By removing second declaration there's a save of 1169 gas for calling function transferWithdrawReserve() using testWithdrawLiquidatedNoBids()

Remove unused local variable.

Remove local variables declared and not used in the function to save gas upon deploying and function calls https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/src/ClearingHouse.sol#L139 - ILienToken.AuctionStack[] storage stack = s.auctionStack.stack; https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L632 - uint256 end = stack[position].end; https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L138 address tokenContract = underlying.tokenContract; https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L139 uint256 tokenId = underlying.tokenId; https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L342 address tokenContract = underlying.tokenContract; https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L262 uint256 assets = totalAssets();

Check "if (amount > owed) amount = owed;" is not needed because repeated in the same function

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L814 At above mentioned line it's checked that if (amount > owed) and if TRUE amount = owed; At line 826 stack.point.amount = owed.safeCastTo88(); amount variable is not used until L829 where there's the if statement if (stack.point.amount > amount) which has the same result of if (owed.safeCastTo88() > amount). If this check is FALSE amount = stack.point.amount; which is equivalent to amount = owed; Therefore one statement between the two in L814 or L836 is redundant