Test: Initial Strategy & Service#3
Conversation
| ) external onlyStrategyOwner(strategyId) { | ||
| if (obligationPercentage > MAX_PERCENTAGE) revert ICore.InvalidPercentage(); | ||
| if (obligationPercentage <= obligations[strategyId][bApp][token]) revert ICore.InvalidPercentage(); | ||
|
|
There was a problem hiding this comment.
Please check new condition when going from 0% to >0%
if (obligations[strategyId][bApp][token] == 0 && obligationPercentage > 0) {
usedTokens[strategyId][token] += 1;
}
or add a check if the obligation does not exist
There was a problem hiding this comment.
Good find Marco!
| obligations[strategyId][bApp][token] = obligationPercentage; | ||
| } | ||
|
|
||
| obligationsCounter[strategyId][bApp] += 1; |
There was a problem hiding this comment.
I think obligationsCounter should be incremented only if obligationPercentage != 0, as it represents the total obligations created in a strategy, right?
maybe also don't emit the next event. So:
if (obligationPercentage != 0) {
usedTokens[strategyId][token] += 1;
obligations[strategyId][bApp][token] = obligationPercentage;
}
obligationsCounter[strategyId][bApp] += 1;
emit ObligationCreated(strategyId, bApp, token, obligationPercentage);
| /// @notice Withdraw ETH from the strategy | ||
| /// @param strategyId The ID of the strategy | ||
| /// @param amount The amount to withdraw | ||
| function fastWithdrawETH(uint256 strategyId, uint256 amount) external { |
There was a problem hiding this comment.
Potential candidate to use nonReentrant modifier.
We need to add a test to simulate a reentrancy attack.
There was a problem hiding this comment.
Yes, I was planning to have timebox task to try and break the contract, testing reentrancy and more.
| * If the counter is 0, the token is unused and we can allow fast withdrawal. | ||
| */ | ||
| mapping(uint256 strategyId => mapping(address token => uint32 servicesCounter)) public usedTokens; | ||
| mapping(uint256 strategyId => mapping(address token => uint32 bAppsCounter)) public usedTokens; |
There was a problem hiding this comment.
Please double check each action that sets an obligation from >0 to 0 or 0 to >0 also updates usedTokens
4 participants
No description provided.