Gro Protocol contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function setWithdrawHandler allows the setting of withdrawHandler and emergencyHandler. However emergencyHandler isn't checked for 0 (like the withdrawHandler ) The value of the emergencyHandler is also not emitted (like the withdrawHandler )

Proof of Concept

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L105 function setWithdrawHandler(address _withdrawHandler, address _emergencyHandler) external onlyOwner { require(_withdrawHandler != address(0), "setWithdrawHandler: 0x"); withdrawHandler = _withdrawHandler; emergencyHandler = _emergencyHandler; emit LogNewWithdrawHandler(_withdrawHandler); }

Tools Used

Recommended Mitigation Steps

Add something like: require(_emergencyHandler!= address(0), "setEmergencyHandler: 0x"); event LogNewEmergencyHandler(address tokens); emit LogNewEmergencyHandler(_emergencyHandler);

Handle

gpersoon

Vulnerability details

Impact

The values of lastRatio in the contract Buoy3Pool are not initialized (thus they have a value of 0). If safetyCheck() would be called before the first time _updateRatios is called, then safetyCheck() would give unexpected results.

Proof of Concept

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pools/oracle/Buoy3Pool.sol#L25 contract Buoy3Pool is FixedStablecoins, Controllable, IBuoy, IChainPrice { ... mapping(uint256 => uint256) lastRatio;

function safetyCheck() external view override returns (bool) { for (uint256 i = 1; i < N_COINS; i++) { uint256 _ratio = curvePool.get_dy(int128(0), int128(i), getDecimal(0)); _ratio = abs(int256(_ratio - lastRatio[i])); if (_ratio.mul(PERCENTAGE_DECIMAL_FACTOR).div(CURVE_RATIO_DECIMALS_FACTOR) > BASIS_POINTS) { return false; } } return true; }

function _updateRatios(uint256 tolerance) private returns (bool) { ... for (uint256 i = 1; i < N_COINS; i++) { lastRatio[i] = newRatios[i];

Tools Used

Recommended Mitigation Steps

Double check if this situation can occur. Perhaps call _updateRatios as soon as possible. Or check in safetyCheck that the lastRatio values are initialized

Handle

gpersoon

Vulnerability details

Impact

The function setUnderlyingTokenPercent doesn't check that the sum of all the percentages is 100%. This way the percentages could be accidentally set up the wrong way, with unpredictable results.

Note: the function can only be called by controller or the owner so the likely hood of mistakes is pretty low.

Proof of Concept

//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Insurance.sol#L100 function setUnderlyingTokenPercent(uint256 coinIndex, uint256 percent) external override onlyValidIndex(coinIndex) { require(msg.sender == controller || msg.sender == owner(), "setUnderlyingTokenPercent: !authorized"); underlyingTokensPercents[coinIndex] = percent; emit LogNewTargetAllocation(coinIndex, percent); }

Tools Used

Recommended Mitigation Steps

Change setUnderlyingTokenPercent to set the percentages for all the coins at the same time. And check that the sum of the percentages is 100%

Handle

gpersoon

Vulnerability details

Impact

Safemath is used on several places but not everywhere. Especially on risky places like PnL and distributeStrategyGainLoss it is hardly worth the gas-savings of not using safemath.

In distributeStrategyGainLoss it does make a difference, also due to another issue.

Proof of Concept

https://github.com/code-423n4/2021-06-gro/blob/main/contracts/pnl/PnL.sol#L215 function handleLoss( uint256 gvtAssets, uint256 pwrdAssets, uint256 loss) private pure returns (uint256, uint256) { uint256 maxGvtLoss = gvtAssets.sub(DEFAULT_DECIMALS_FACTOR); if (loss > maxGvtLoss) { ... } else { gvtAssets = gvtAssets - loss; // won't underflow but safemath won't hurt }

function forceDistribute() private {
    uint256 total = _controller().totalAssets();
    if (total > lastPwrdAssets.add(DEFAULT_DECIMALS_FACTOR)) {
        lastGvtAssets = total - lastPwrdAssets;   // won't underflow but safemath won't hurt
    } else {

...

//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/Controller.sol#L355 function distributeStrategyGainLoss(uint256 gain, uint256 loss) external override { uint256 index = vaultIndexes[msg.sender]; require(index > 0 || index <= N_COINS + 1, "!VaultAdaptor"); // always true, see separate issue .. index = index - 1; // can underflow

Tools Used

Recommended Mitigation Steps

Apply safemath or move to Solidity 0.8.x

Handle

gpersoon

Vulnerability details

Impact

The contract BaseVaultAdaptor contains the function migrate which can be used to move all the ERC20 tokens to another contract. This can be seen as a rug pull Even if this is well intended the project could still be called out, see for example: https://twitter.com/RugDocIO/status/1408097542202531840

Proof of Concept

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/BaseVaultAdaptor.sol#L296 function migrate(address child) external onlyOwner { require(child != address(0), "migrate: child == 0x"); IERC20 _token = IERC20(token); uint256 balance = _token.balanceOf(address(this)); _token.safeTransfer(child, balance); emit LogMigrate(address(this), child, balance); }

Tools Used

Recommended Mitigation Steps

Remove the code or apply extra safeguards

Handle

gpersoon

Vulnerability details

Impact

In several functions of BaseVaultAdaptor a value is stored in the variable amount at the end of the function. However this variable is never used afterwards so the storage is unnecessary and just uses gas.

Proof of Concept

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/vaults/BaseVaultAdaptor.sol#L165 function withdraw(uint256 amount) external override { .. if (!_withdrawFromAdapter(amount, msg.sender)) { amount = _withdraw(calculateShare(amount), msg.sender);

function withdraw(uint256 amount, address recipient) external override {
...
    if (!_withdrawFromAdapter(amount, recipient)) {
        amount = _withdraw(calculateShare(amount), recipient);

function withdrawToAdapter(uint256 amount) external onlyOwner {
    amount = _withdraw(calculateShare(amount), address(this));
}

function withdrawByStrategyOrder(

.. if (!_withdrawFromAdapter(amount, recipient)) { amount = _withdrawByStrategyOrder(calculateShare(amount), recipient, reversed);

function withdrawByStrategyIndex(

... if (!_withdrawFromAdapter(amount, recipient)) { amount = _withdrawByStrategyIndex(calculateShare(amount), recipient, strategyIndex);

Tools Used

Recommended Mitigation Steps

Replace amount = _withdraw***(...); with _withdraw***(...);

Handle

gpersoon

Vulnerability details

Impact

calcProtocolExposureDelta should probably stop executing once it has found the first occurrence where exposure > threshold. (as is also indicated in the comment).

The current code also works (due to the check protocolExposedDeltaUsd == 0), however inserting a break statement at the end of the "if" is more logical and saves a bit of gas.

Proof of Concept

//https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Allocation.sol#L286 /// By defenition, only one protocol can exceed exposure in the current setup. ... function calcProtocolExposureDelta(uint256[] memory protocolExposure, SystemState memory sysState) private pure returns (uint256 protocolExposedDeltaUsd, uint256 protocolExposedIndex) { for (uint256 i = 0; i < protocolExposure.length; i++) { // If the exposure is greater than the rebalance threshold... if (protocolExposedDeltaUsd == 0 && protocolExposure[i] > sysState.rebalanceThreshold) { // ...Calculate the delta between exposure and target uint256 target = sysState.rebalanceThreshold.sub(sysState.targetBuffer); protocolExposedDeltaUsd = protocolExposure[i].sub(target).mul(sysState.totalCurrentAssetsUsd).div( PERCENTAGE_DECIMAL_FACTOR ); protocolExposedIndex = i;
// probably put a break here } } }

Tools Used

Recommended Mitigation Steps

Add a break statement at the end of the if

Handle

gpersoon

Vulnerability details

Impact

In the function applyFactor of GToken.sol there seems to be an attempt on gas optimization by doing: uint256 _BASE = BASE;

However because BASE is a constant it is cheaper not to do this and just use BASE.

A minor gas optimization is possible by storing a.mul(b) or a.mul(_BASE) in temporary variable because it is evaluated twice.

Proof of Concept

https://github.com/code-423n4/2021-06-gro/blob/main/contracts/tokens/GToken.sol uint256 public constant BASE = DEFAULT_DECIMALS_FACTOR; function applyFactor(uint256 a,uint256 b, bool base ) internal pure returns (uint256 resultant) { uint256 _BASE = BASE; uint256 diff; if (base) { diff = a.mul(b) % _BASE; resultant = a.mul(b).div(_BASE); } else { diff = a.mul(_BASE) % b; resultant = a.mul(_BASE).div(b); } if (diff >= 5E17) { resultant = resultant.add(1); } }

Tools Used

Recommended Mitigation Steps

Use BASE instead of _BASE

Perhaps use the following optimization: if (base) { uint256 tmp=a.mul(b) ; diff = tmp % _BASE; resultant = tmp.div(_BASE); } else { uint256tmp=a.mul(_BASE) ; diff = tmp % b; resultant = tmp.div(b); }

Handle

gpersoon

Vulnerability details

Impact

The parameters maxPercentForWithdraw and maxPercentForDeposit are not directly initialized. If functions, which rely on these parameters, are called, before setWhaleThresholdWithdraw/setWhaleThresholdDeposit, then they will work in a suboptimal way.

Proof of Concept

// https://github.com/code-423n4/2021-06-gro/blob/main/contracts/insurance/Insurance.sol#L63 uint256 public maxPercentForWithdraw; uint256 public maxPercentForDeposit;

function setWhaleThresholdWithdraw(uint256 _maxPercentForWithdraw) external onlyOwner { maxPercentForWithdraw = _maxPercentForWithdraw; emit LogNewVaultMax(false, _maxPercentForWithdraw); } function setWhaleThresholdDeposit(uint256 _maxPercentForDeposit) external onlyOwner { maxPercentForDeposit = _maxPercentForDeposit; emit LogNewVaultMax(true, _maxPercentForDeposit); }

Tools Used

Recommended Mitigation Steps

Assign a default value to maxPercentForWithdraw and maxPercentForDeposit or initialize the values via the constructor.