Summary
when you call SiloFacet::claimPlenty function will call other function which is import from the Silo::_claimPlenty but if you check those function it transfer token to the account then it update the storage.
Vulnerability Details
Reentrancy Attack it you look closely the function Silo::_claimPlenty it call sopToken.safeTransfer then it delete the storage user can make other call an get more fund from it because it not check the plenty of address is > zero and also it delete after transfer.
* @dev Gas optimization: An account can call `{SiloFacet:claimPlenty}` even
* if `s.a[account].sop.plenty == 0`. This would emit a ClaimPlenty event
* with an amount of 0.
*/
function _claimPlenty(address account) internal {
@> uint256 plenty = s.a[account].sop.plenty;
IWell well = IWell(s.sopWell);
IERC20[] memory tokens = well.tokens();
IERC20 sopToken = tokens[0] != C.bean() ? tokens[0] : tokens[1];
sopToken.safeTransfer(account, plenty);
@> delete s.a[account].sop.plenty;
emit ClaimPlenty(account, address(sopToken), plenty);
}
"description": "Reentrancy in Silo._claimPlenty(address) (contracts/beanstalk/silo/SiloFacet/Silo.sol#154-164):\n\tExternal calls:\n\t- sopToken.safeTransfer(account,plenty) (contracts/beanstalk/silo/SiloFacet/Silo.sol#160)\n\tState variables written after the call(s):\n\t- delete s.a[account].sop.plenty (contracts/beanstalk/silo/SiloFacet/Silo.sol#161)\n",
"markdown": "Reentrancy in [Silo._claimPlenty(address)](contracts/beanstalk/silo/SiloFacet/Silo.sol#L154-L164):\n\tExternal calls:\n\t- [sopToken.safeTransfer(account,plenty)](contracts/beanstalk/silo/SiloFacet/Silo.sol#L160)\n\tState variables written after the call(s):\n\t- [delete s.a[account].sop.plenty](contracts/beanstalk/silo/SiloFacet/Silo.sol#L161)\n",
"first_markdown_element": "contracts/beanstalk/silo/SiloFacet/Silo.sol#L154-L164",
"id": "4dbd56b6b33173000ad84687d65d9177a0ce83b578ae9abf20102990fe8648e1",
"check": "reentrancy-no-eth",
"impact": "Medium",
"confidence": "Medium"
Impact
Lose fund by Attacker call this function again and again.
Tools Used
Manual Review.
Recommendations
Delete storage before transfer and add check plenty > 0 to not waste gas try to transfer zero amount.
/**
* @dev Gas optimization: An account can call `{SiloFacet:claimPlenty}` even
* if `s.a[account].sop.plenty == 0`. This would emit a ClaimPlenty event
* with an amount of 0.
*/
function _claimPlenty(address account) internal {
// Plenty is earned in the form of the sop token.
uint256 plenty = s.a[account].sop.plenty;
+ required(plenty>0,"No plenty AMount");
IWell well = IWell(s.sopWell);
IERC20[] memory tokens = well.tokens();
IERC20 sopToken = tokens[0] != C.bean() ? tokens[0] : tokens[1];
+ delete s.a[account].sop.plenty;
sopToken.safeTransfer(account, plenty);
- delete s.a[account].sop.plenty;
// q: based in descritopn event will only called when plenty==0 but i didn't see any check
emit ClaimPlenty(account, address(sopToken), plenty);
}
