CWE-834: Excessive Iteration (UPDATE: enrootDeposit)
Report ID
#13942
Target
Report type
Smart Contract
Impacts
Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
Has PoC?
Yes
Bug Description
The excessive calling of function enrootDeposit seems to be unnecessary if I understand the logic of the SiloFacet.sol and the enrootDeposit function proberly.
Allowance of excessive iteration causes following documented attack vectors: CWE-834: Excessive Iteration and possibly CWE-835: Loop with Unreachable Exit Condition ('Infinite Loop')
Following the logic definition of enroot:
"Enroot- Adds Revitalized Stalk and Seeds to your Stalk and Seed balances, respectively. Revitalized Stalk - Stalk that have vested for pre-exploit Silo Members. Revitalized Stalk are minted as the percentage of Fertilizer sold increases. Revitalized Stalk does not contribute to Stalk ownership until Enrooted.
Revitalized Assets- Upon Replant, Silo Members in the block prior to the exploit received a portion of their Stalk and Seeds based on the percentage of Fertilizer sold prior to Replant. As the percentage of Fertilizer sold increases, additional Stalk and Seeds become Revitalized and can be Enrooted. Revitalized Stalk and Seeds start earning Bean seigniorage and Grown Stalk, respectively, upon being Enrooted." source https://docs.bean.money/guides
If I understand the above right enroot for direct deposits can only be done once per season to add revitalized stalk and seed to balances. There is no explanation why it should be called multiple times for same args, or rather does not make sense. Unless, enroot can be called with same args multiple times for stalk ownership
Impact
"If the iteration can be influenced by an attacker, this weakness could allow attackers to consume excessive resources such as CPU or memory. In many cases, a loop does not need to be infinite in order to cause enough resource consumption to adversely affect the software or its host system; it depends on the amount of resources consumed per iteration." - CWE-834: Excessive Iteration
Risk Breakdown
Difficulty to Exploit: Easy
Recommendation
Calling enrootDeposit, excessively does not add any value to the overall logic i.e. using (same arguments) after first call since per the logic of the program the enrootDeposit can only be done once to revitalized seed and stalk.
Or alternatively function Deposit() using External mode functionality should be removed since no one has used it after the exploit (please check diamond on chain activity)
References
https://cwe.mitre.org/data/definitions/834.html https://cwe.mitre.org/data/definitions/835.html https://arxiv.org/pdf/2107.11598.pdf https://help.nintex.com/en-us/nintex2010/help/workflow/RootCategory/Configuration/Nintex.Workflow.PreventingExcessiveLooping.htm
NB: Like I said, for a skilled hacker this is a valid entry point to cause infinite loop or undesired behavior. Stand to be corrected
Proof of concept
pragma solidity 0.8.10;
import "forge-std/Test.sol";
import "forge-std/console.sol";
/*Key Information
Attack Name : Multi-Enroot
Summary : Enroot can be called multiple times in same season
Risk Assessment : Critical
Vulnerable Contract : Diamond -> SiloFacet.sol
Vulnerable Function(s) : Function enrootDeposit
Vulnerable Type : Multiple Call
Tools Used : Sol2UML, VS-Surya, Foundry and Slither
Research links of Vulnerability : N/A
POC-Written by : Sentient-X
twitter : @sentient_x
Disclaimer - This POC is the product of the authors best understanding of the project and associated vulnerability all claims to be proven.
*/
address constant BEAN_POOL = 0xc9C32cd16Bf7eFB85Ff14e0c8603cc90F6F2eE49; //CURVE_BEAN_METAPOOL
address constant DIAMOND = 0xC1E088fC1323b20BCBee9bd1B9fC9546db5624C5;
address constant BEAN3CRV = 0xc9C32cd16Bf7eFB85Ff14e0c8603cc90F6F2eE49; //CURVE_BEAN_METAPOOL
//Bean tokens
address constant BEAN_LP = 0x1BEA3CcD22F4EBd3d37d731BA31Eeca95713716D; //urBEAN3CRV
address constant BEAN = 0xBEA0000029AD1c77D3d5D23Ba2D8893dB9d1Efab; //BEAN Token
address constant urBEAN = 0x1BEA0050E63e05FBb5D8BA2f10cf5800B6224449; //urBEAN Token
address constant Crv = 0x6c3F90f043a72FA612cbac8115EE7e52BDe6E490; //Curve.fi DAI/USDC/USDT (3Crv)
address constant USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
contract ContractTest is Test {
ICurve private constant pool = ICurve(BEAN_POOL);
IDIAMOND private constant diamond = IDIAMOND(DIAMOND);
IERC20 public constant lpToken = IERC20(BEAN_LP);
IERC20 public constant bean = IERC20(BEAN);
IERC20 public constant urbean = IERC20(urBEAN);
IERC20 public constant crv = IERC20(Crv);
IERC20 public constant bean2 = IERC20(BEAN3CRV);
IERC20 public constant usdc = IERC20(USDC);
address alice = vm.addr(1);
address user = 0x3a7268413227Aed893057a8eB21299b36ec3477e;
function setUp() public {
vm.createSelectFork("mainnet",15973462); //fork mainnet at block 15948124
}
function testDeposit() external payable {
//deal address(this) 1 bean
deal(address(bean), address(this), 1000e18);
assertEq(bean.balanceOf(address(this)), 1000e18);
bean.approve(address(diamond), type(uint).max);
diamond.approveDeposit(address(bean), address(this), type(uint).max);
diamond.deposit(address(bean), 1000e18, 2);
//expect this enroot loop to revert, since the deposit in season 8487 is already enrooted
for(uint i = 1; i < 9999; i++){
(bool success,) = address(diamond).call(abi.encodeWithSignature("enrootDeposit(address,uint32,uint256)", address(bean), 8487, 1000e18));
require(success);
}
}
interface ICurve {
function get_virtual_price() external view returns (uint);
function remove_liquidity(uint lp, uint[2] calldata min_amounts)
external
returns (uint[2] memory);
function add_liquidity(uint[2] calldata _amounts, uint _min_mint_amount)
external
payable
returns (uint);
function remove_liquidity_one_coin(
uint lp,
int128 i,
uint min_amount
) external returns (uint);
function exchange_underlying(int128 i, int128 j, uint256 dx, uint256 min_dy) external;
}
interface IERC20 {
function totalSupply() external view returns (uint);
function balanceOf(address account) external view returns (uint);
function transfer(address recipient, uint amount) external returns (bool);
function allowance(address owner, address spender) external view returns (uint);
function approve(address spender, uint amount) external returns (bool);
function transferFrom(
address sender,
address recipient,
uint amount
) external returns (bool);
event Transfer(address indexed from, address indexed to, uint value);
event Approval(address indexed owner, address indexed spender, uint value);
}
interface IDIAMOND {
function approveDeposit(address token, address account, uint256 amount) external;
function deposit(address token ,uint256 amount , uint8 mode) external;
function withdrawDeposit(address token, uint32 season, uint256 amount) external;
function updateSilo(address account) external;
function convert(bytes calldata convertData, uint32[] memory crates, uint256[] memory amounts) external;
function withdrawDeposits(address token, uint32[] calldata seasons, uint256[] calldata amounts) external;
function buyAndDepositBeans(uint256 amount, uint256 buyAmount) external;
function plant() external;
function enrootDeposit(address token, uint32 _season, uint256 amount) external;
function harvest(uint256[] calldata plots) external;
function mintFertilizer(uint128 amount, uint256 minLP, uint8 mode) external;
function chop(address token, uint256 amount, uint8, uint8) external;
function balanceOfStalk(address account) external;
function claimWithdrawal(address, uint32, uint8) external;
function claimPlenty() external;
function withdrawFreeze() external;
function transferDeposit(address sender, address recipient, address token, uint32 season, uint256 amount) external;
function sunrise() external;
function owner() external;
function balanceOfSeeds(address account) external;
}```BIC Response
This is not a security bug report because repeated calling of enrootDeposit(s) is expected behavior. Any calls to enrootDeposit(s) that do not update the Deposit's BDV to a higher value would simply be wasting gas.
If I understand the above right enroot for direct deposits can only be done once per season to add revitalized stalk and seed to balances
This is incorrect, as the BDV of Unripe Deposits can change at any time.
It also unclear what the attack vector is in this bug report.
Due to these reasons, we are closing the submission and no reward will be issued.