Aave DIVA Wrapper
DIVA
HardhatDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Anybody can pass other's `poolId` which will cause harm to the actual `poolId` owner

sakibcy

Summary

There is no validation in Aave DIVA Wrapper and also in Diva Protocol to check if the passed poolId belongs to the correct owner.
As a result an attacker who has intention to harm a user can call lets say removeLiquidity and pass a poolId which will reduce funds from the passed poolId.

!!!! Note: As Aave DIVA Wrapper is integretaded with Diva Protocol, so we the auditors are not thinking of the validity check on Diva protocol here, but there should be a validity check on the Aave DIVA Wrapper, because it is now in the scope of this contest.

Vulnerability Details

The AaveDIVAWrapperCore::_removeLiquidity takes poolId as parameter then it call IDIVA(_diva).removeLiquidity(_poolId, _positionTokenAmountToRemove);.

function _removeLiquidity(
bytes32 _poolId,
uint256 _positionTokenAmount,
address _recipient
) internal returns (uint256) {
// Query pool parameters to obtain the collateral token as well as the
// short and long token addresses.
IDIVA.Pool memory _pool = IDIVA(_diva).getPoolParameters(_poolId);
​
// ...OTHER_CODES...
​
235:: IDIVA(_diva).removeLiquidity(_poolId, _positionTokenAmountToRemove);
​
// ...OTHER_CODES...
}

On the LibDIVA::_removeLiquidityLib of Diva Protocol we can see it is reducing fee from the passed poolId.

function _removeLiquidityLib(
RemoveLiquidityParams memory _removeLiquidityParams,
LibDIVAStorage.Pool storage _pool
) internal returns (uint256 collateralAmountRemovedNet) {
// Get reference to relevant storage slot
LibDIVAStorage.GovernanceStorage storage gs = LibDIVAStorage
._governanceStorage();
​
// ...OTHER_CODES...
​
951:: // Allocate protocol fee to DIVA treasury. Fee is held within this
// contract and can be claimed via `claimFee` function.
// `collateralBalance` is reduced inside `_allocateFeeClaim`.
_allocateFeeClaim(
_removeLiquidityParams.poolId,
_pool,
_getCurrentTreasury(gs),
_protocolFee
);
​
// Reserve settlement fee for data provider which is not known at this stage.
// Fee will be allocated to actual data provider following final value
// confirmation and afterwards can be claimed via the `claimFee` function.
_reserveFeeClaim(
_removeLiquidityParams.poolId,
_pool,
_settlementFee
);
// Collateral amount to return net of fees
collateralAmountRemovedNet =
_removeLiquidityParams.amount -
_protocolFee -
974:: _settlementFee;
​
// ...OTHER_CODES...
}

So it is calling _reserveFeeClaim and _allocateFeeClaim which will reduce amount for fee from the passed poolId -> pool.

Impact

The poolId owner will lose funds as there is no validation for the poolId.

Though there are some logic on the contracts that will check msg.sender's balance and will transfer balances from msg.sender. But ultimately it reduce lets say fee amount from the passed poolId.

So the actual owner of the poolId's balance will be reduced. If a lot of attacker passed his poolId then his balance will be reduce in huge number.

Tools Used

Manual review

Recommendations

Add validation on functions that poolId belongs to correct owner which has poolId as parameter.
Lets say the dataProvider is the owner of poolId, so check if the caller (msg.sender) of a function which has poolId is the actual dataProvider.

Updates

Lead Judging Commences

bube Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.