The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Low Risk Findings

spacelord47

[L-01] SmartVaultManagerV5 implementation contract allows unauthorized initialization

Summary

SmartVaultManagerV5 contract utilizes Initializable contract from OpenZeppelin(OZ) library, but doesn't disable the initializer function, to prevent unauthorized initialization of the implementation contract (deployed behind a proxy).

Issue Details

SmartVaultManagerV5 uses the "transparent upgradeable proxy" pattern, and each version of the contract (implementation) is intended to be deployed behind a proxy.
The initialize function, expected to be called (by the deploy script in this case) after each contract upgrade. But during deploy it will be called only for the proxy contract( and not implementation).

The documentation for Initializable OZ contract clearly states that initialization functionality should be manually disabled for implementation contracts. But in SmartVaultManagerV5 it's not implemented.

Impact

Even though there is no way to exploit this flaw in the current version of SmartVaultManager (because the initializer function is empty), it poses serious risks for any further versions of this contract, which might add logic to the initializer function, but omit safeguarding it from malicious actors.

Also, there is no reason to ignore strict recommendations from the OZ documentation.

PoC

The following test case demonstrates how a unauthorized user can initialize the implementation contract for SmartVaultManagerV5

diff --git a/test/smartVaultManager.js b/test/smartVaultManager.js
--- a/test/smartVaultManager.js (revision c12272f2eec533019f2d255ab690f6892027f112)
+++ b/test/smartVaultManager.js (date 1703763496571)
@@ -33,6 +33,38 @@
await EUROs.grantRole(await EUROs.DEFAULT_ADMIN_ROLE(), VaultManager.address);
});
+ it.only("allows unauthorized user to initialize implementation contract", async () => {
+ // Fetch the implementation address by reading proxy storage slot at keccak256("eip1967.proxy.implementation") - 1
+ const implAddress = await ethers.provider
+ .getStorageAt(
+ VaultManager.address,
+ ethers.BigNumber.from("0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc"))
+ .then((implAtStorage) => `0x${implAtStorage.slice(-40)}`);
+
+ // Now read private variable "_initialized"(at slot "0") from implementation contract
+ const isImplInitialized = await ethers.provider
+ .getStorageAt(implAddress, 0)
+ .then((storageSlot) => ethers.BigNumber.from(storageSlot));
+
+ // Confirm that the implementation contract is not initialized
+ expect(isImplInitialized).to.equal(0);
+
+ const [unauthorizedSigner] = await ethers.getSigners();
+
+ // A unauthorized user performs call to "initialize"
+ await unauthorizedSigner.sendTransaction({
+ to: implAddress,
+ data: "0x8129fc1c", // "initialize()
+ });
+
+ const isImplInitialized2 = await ethers.provider
+ .getStorageAt(implAddress, 0)
+ .then((storageSlot) => ethers.BigNumber.from(storageSlot));
+
+ // Confirm that the contract now initialized
+ expect(isImplInitialized2).to.equal(1);
+ });
+
describe('setting admin data', async () => {
it('allows owner to admin data', async () => {
expect(await VaultManager.mintFeeRate()).to.equal(PROTOCOL_FEE_RATE);

Run with npx hardhat test

Recommendations

Use _disableInitializers function in the constructor to prevent initialization of the implementation contract.

diff --git a/contracts/SmartVaultManagerV5.sol b/contracts/SmartVaultManagerV5.sol
--- a/contracts/SmartVaultManagerV5.sol (revision c12272f2eec533019f2d255ab690f6892027f112)
+++ b/contracts/SmartVaultManagerV5.sol (date 1703763214093)
@@ -43,6 +43,11 @@
uint256 burnFeeRate; ISmartVault.Status status;
}
+ /// @custom:oz-upgrades-unsafe-allow constructor
+ constructor() {
+ _disableInitializers();
+ }
+
function initialize() initializer public {}
modifier onlyLiquidator {

[L-02] ERC20 token approval not reset for LiquidationPool if there was no fee distribution

Summary

During fee distribution, LiqudationPoolManager approves LiqudationPool to distribute EUROs tokens among all stakers. But LiquidationPool will not spend this allowance, if there is no TST token staked.

Issue Details

LiquidationPoolManager holds EUROs tokens which is distributed between stakers and the Protocol.
LiquidationPoolManager.distributeFees function called by LiquidationPool during fee distribution:
contracts/LiquidationPoolManager.sol

33: function distributeFees() public {
34: IERC20 eurosToken = IERC20(EUROs);
35: uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
36: if (_feesForPool > 0) {
37: eurosToken.approve(pool, _feesForPool);
38: LiquidationPool(pool).distributeFees(_feesForPool);
39: }
40: eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
41: }

In this function, LiquidationPoolManager approves LiquidationPool to spend EUROs, if there is some EUROs collected.
LiquidationPool.distributeFees function uses this allowance to transfer EUROs to stakers:

contracts/LiquidationPool.sol

182: function distributeFees(uint256 _amount) external onlyManager {
183: uint256 tstTotal = getTstTotal();
184: if (tstTotal > 0) {
185: IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
186: for (uint256 i = 0; i < holders.length; i++) {
187: address _holder = holders[i];
188: positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
189: }
190: for (uint256 i = 0; i < pendingStakes.length; i++) {
191: pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
192: }
193: }
194: }

As we can see, EUROs will be transferred only if there is some TST tokens staked. That means, if tstTotal == 0, the allowance will not be spent, and LiquidationPool will be able to spend EUROs held by LiquidationPoolManager even after fee distribution function completed.

Impact

This issue by itself doesn't harm the Protocol, but it might be exploited though vulnerability chaining in another function (I've reported another vulnerability in distributeAssets function which is an ideal candidate for this scenario).
So, there is no reasons to leave this gap in the logic.

Recommendation

Consider the following options to resolve the issue:

  1. If getTstTotal function can be made public, it can be utilized in LiquidationPoolManager.distributeFees to prevent approval and call to LiquidationPool.distributeFees, if tstTotal == 0

  2. LiquidationPool.distributeFees can return a bool, to signal whether fee distribution actually happened, so LiquidationPoolManager can reset approval if needed

  3. Reset approval each time (NOTE: this is less desirable method because some tokens may revert on zero approvals):

36: if (_feesForPool > 0) {
37: eurosToken.approve(pool, _feesForPool);
38: LiquidationPool(pool).distributeFees(_feesForPool);
39: eurosToken.approve(pool, 0); // @audit Reset approval
40: }
41: eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

informational/invalid

Support

FAQs

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