BaseStrategy implements USTStrategy and is risky to inherit from

[code423n4]

Handle

palina

Vulnerability details

Impact

Despite the title suggesting that it should serve as a "template" and implement common basic functionality, BaseStrategy.sol contains a rather concrete implementation of the USTStrategy.

This code organization might be dangerous since the implementation of UST- and BaseStrategy assumes that the underlying token and UST token are the same. Therefore, in BaseStrategy's investedAssets(), the underlyingBalance is calculated as a sum of the contract balance and pendingDeposits (which are both in UST only if it's a USTStrategy, and it would be wrong to sum up otherwise).

Although the implementation of the NonUSTStrategy.sol handles this case correctly, new strategies inheriting from BaseStrategy might miss the re-implementation of investedAssets() given in the BaseStrategy.sol leading to flawed calculation.

Proof of Concept

BaseStrategy.investedAssets(): https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/BaseStrategy.sol#L270

NonUSTStrategy.investedAssets(): https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/NonUSTStrategy.sol#L126

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider moving the detailed implementation of UST/NonUST-specific functionality from BaseStrategy to USTStrategy, leaving only abstract and shared functionality in BaseStrategy.

[gabrielpoca]

@ryuheimat I disagree with this being a med risk strategy. There's no exploit here.

[r2moon]

@gabrielpoca i agree with you. This does not have the risk,

[dmvt]

This is a code style / clarity issue IMO. The warden has a point, but I don't see any scenario where funds are at risk.