Submitted by hansfriese, also found by oyc109, xiaoming90, kenzo, Kumpa, unforgiven, MiloTruck, and pauliax_
Strategists can take more rewards than they should using the function strategistBootyClaim().
Even though the owner trusts strategists fully I think its recommended to remove such flaws.
I think there would be 2 methods to claim more rewards.
A strategist can call the function using same asset/quote parameters.
Then both of fillCountA and fillCountQ will be same positive values.
The first code block for fillCountA(L597-L610) will work same as expected but the second block for fillCountQ(L611-L624) will be executed for the same asset again.
Two mappings(totalFillsPerAsset, strategist2Fills) that save rewards will be updated for asset already after the first block
but totalFillsPerAsset and balance of this contract for quote would be still positive as there would be remaining rewards for other strategiets.
So the strategist can get paid once more for the same asset.
I think a reentrancy attack is possible also because two mappings are updated after transfer funds.
Solidity Visual Developer of VSCode
You can add this require() at the beginning of function.(L595)
require(asset != quote, asset = quote);
You can update the state of 2 mappings before transfer.
Move L608-L609 to L601
Move L622-L623 to L615
So final code will look like this.(pseudocode)
function strategistBootyClaim(address asset, address quote)
external
onlyApprovedStrategist(msg.sender)
{
require(asset != quote, asset = quote);
}
bghughes (Rubicon) confirmed
KenzoAgada (warden) commented:
HickupHH3 (judge) commented:
HickupHH3 (judge) commented:
