Astaria contest - fs0c's results

Lines of code

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

Vulnerability details

In _paymentAH function from LienToken.sol, the stack argument should be storage instead of memory. This bug was also disclosed in the Spearbit audit of this program and was resolved during here: https://github.com/AstariaXYZ/astaria-core/pull/201/commits/5a0a86837c0dcf2f6768e8a42aa4215666b57f11, but was later re-introduced https://github.com/AstariaXYZ/astaria-core/commit/be9a14d08caafe125c44f6876ebb4f28f06d83d4 here. Marking it as high-severity as it was marked as same in the audit.

POC

function _paymentAH(
    LienStorage storage s,
    address token,
    AuctionStack[] memory stack,
    uint256 position,
    uint256 payment,
    address payer
  ) internal returns (uint256) {
    uint256 lienId = stack[position].lienId;
    uint256 end = stack[position].end;
    uint256 owing = stack[position].amountOwed;
    //...[deleted lines to show bug]
    
    delete s.lienMeta[lienId]; //full delete
    delete stack[position]; // <- no effect on storage
    
  }

The position here is not deleted after the debt is paid as it is a memory pointer.

Recommendation

The first fix can be used again and it would work.

Lines of code

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

Vulnerability details

Impact

The actual transfer might be less that the amount transfered. This would lead to wrong accounting in the contract.

Vulnerability Details

Eg: In _paymentAH function, if the token is a fee-on-transfer token then the actual tokens transferred from payer to payee would be less than payment , but the payee should receive the full amount.

if (payment > 0)
      s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);

Recommended Mitigation Steps

Transfer the tokens first and compare pre-/after token balances to compute the actual deposited amount.

[G-01] Can stop loop early in _payDebt when everything is spent.

When a loan is sold on Seapost and _payDebt is called, it loops thought the auction stack and calls _paymentAH for each, decrementing the remaining payment as money is spent.

This loop can be ended when payment == 0.

Recommendation

uint256 i;
     for (; i < stack.length;) {
+      if (payment == 0) break;
       uint256 spent;
       unchecked {
         spent = _paymentAH(s, token, stack, i, payment, payer);

[G-02] Remove unused variable.

1. The local variable in function LientToken.sol:_paymentAH can be removed as it is not used.

uint256 end = stack[position].end;

2. The local variable in function CollateralToken.sol:releaseToAddress can be removed as it is not used

    address tokenContract = underlying.tokenContract;

3. CollateralToken.sol:liquidatorNFTClaim

address tokenContract = underlying.tokenContract;
uint256 tokenId = underlying.tokenId;

Both tokenContract and tokenId are unused.

[G-03] Unnecessary use of mulDivDown.

In PublicVault.sol

function _totalAssets(VaultData storage s) internal view returns (uint256) {
    uint256 delta_t = block.timestamp - s.last;
    return uint256(s.slope).mulDivDown(delta_t, 1) + uint256(s.yIntercept);
  }

uint256(s.slope).mulDivDown(delta_t, 1) can be change to uint256(s.slope)*delta_t to save gas.