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

The `AaveDIVAWrapper` contract can be bricked after deployment due to inconsistent parameters passing

serialcoder

Summary

The AaveDIVAWrapper::constructor() incorrectly passes parameters to its mother contract, AaveDIVAWrapperCore::constructor(). Consequently, the AaveDIVAWrapper contract can be bricked after deployment.

Vulnerability Details

The AaveDIVAWrapper::constructor() receives the _aaveV3Pool, _diva, and _owner as parameters and respectively passes them (see @1 in the snippet below) to its mother contract, AaveDIVAWrapperCore::constructor().

contract AaveDIVAWrapper is AaveDIVAWrapperCore, ReentrancyGuard {
...
​
//@audit -- The AaveDIVAWrapper::constructor() receives the _aaveV3Pool, _diva, and _owner as parameters and
// respectively passes them to its mother contract, AaveDIVAWrapperCore::constructor().
@1 constructor(address _aaveV3Pool, address _diva, address _owner) AaveDIVAWrapperCore(_aaveV3Pool, _diva, _owner) {}
​
...
}

However, the AaveDIVAWrapperCore::constructor() (i.e., the mother contract) defines its parameters in a different order (refer to @2.1). Instead of receiving the aaveV3Pool_ and then diva_, it expects the opposite order.

Hence, the AaveDIVAWrapperCore::constructor() will eventually assign the immutable variables _diva and _aaveV3Pool erroneously (@2.2), rendering the AaveDIVAWrapper contract to be bricked after deployment.

abstract contract AaveDIVAWrapperCore is IAaveDIVAWrapper, Ownable2Step {
...
​
//@audit -- However, the AaveDIVAWrapperCore::constructor() defines its parameters in a different order.
// Instead of receiving the aaveV3Pool_ and then diva_, it expects the opposite order.
@2.1 constructor(address diva_, address aaveV3Pool_, address owner_) Ownable(owner_) {
// Validate that none of the input addresses is zero to prevent unintended initialization with default addresses.
// Zero address check on `owner_` is performed in the OpenZeppelin's `Ownable` contract.
if (diva_ == address(0) || aaveV3Pool_ == address(0)) {
revert ZeroAddress();
}
​
//@audit -- Hence, the AaveDIVAWrapperCore::constructor() will eventually assign the immutable variables _diva and _aaveV3Pool
// erroneously, rendering the AaveDIVAWrapper contract to be bricked after deployment.
//
// Store the addresses of DIVA Protocol and Aave V3 in storage.
@2.2 _diva = diva_;
@2.2 _aaveV3Pool = aaveV3Pool_;
}
​
...
}

In addition to the test script, we noticed that the setup() wrongly passes the parameters in the opposite order (@3) of the AaveDIVAWrapper::constructor()'s requirement.

This mistake makes the test script runnable, fortunately. That's the reason why the test script cannot detect this mis-implementation.

// Test setup function
async function setup(): Promise<SetupOutput> {
...
​
// Deploy AaveDIVAWrapper contract
const aaveDIVAWrapper: AaveDIVAWrapper = await ethers.deployContract(
"AaveDIVAWrapper",
//@audit -- In addition to the test script, we noticed that the setup() wrongly passes the parameters
// in the opposite order of the AaveDIVAWrapper::constructor()'s requirement.
//
// This mistake makes the test script runnable, fortunately. That's the reason why
// the test script cannot detect this mis-implementation.
@3 [divaAddress, aaveAddress, owner.address],
);
await aaveDIVAWrapper.waitForDeployment();
​
...
}

Impact

Note: this is not about the admin-input mistake issue but the incorrect implementation of the AaveDIVAWrapper::constructor().

The impact is considered low (there are no direct financial problems, but the AaveDIVAWrapper contract cannot operate after deployment with the correct order of parameters).

The likelihood is high (if an admin deploys the contract by inputting arguments to the AaveDIVAWrapper::constructor() in the correct order, the issue will arise).

For this reason, this issue deserves a medium.

Tools Used

Manual Review

Recommendations

Improve the order of parameters of the AaveDIVAWrapper::constructor() to be consistent with that of the AaveDIVAWrapperCore::constructor(), as in the snippet below.

contract AaveDIVAWrapper is AaveDIVAWrapperCore, ReentrancyGuard {
...
​
- constructor(address _aaveV3Pool, address _diva, address _owner) AaveDIVAWrapperCore(_aaveV3Pool, _diva, _owner) {}
+ constructor(address _diva, address _aaveV3Pool, address _owner) AaveDIVAWrapperCore(_diva, _aaveV3Pool, _owner) {}
​
...
}
Updates

Lead Judging Commences

bube Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Constructor arguments mismatch

Support

FAQs

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