DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

A vault can have duplicate bridges which may lead to accounting errors and a loss of funds

dipp

Summary

A vault can have duplicates of the same bridge leading to possible incorrect yield calculations.

Vulnerability Details

The createBridge function in OwnerFacet.sol does not check if a bridge has already been added to a vault allowing duplicates in the vault's vaultBridges array.

function createBridge(
address bridge,
uint256 vault,
uint16 withdrawalFee,
uint8 unstakeFee
) external onlyDAO {
s.vaultBridges[vault].push(bridge); multiple times.
s.bridge[bridge].vault = uint8(vault);
_setWithdrawalFee(bridge, withdrawalFee);
_setUnstakeFee(bridge, unstakeFee);
emit Events.CreateBridge(bridge, s.bridge[bridge]);
}

In LibVault.sol, the getZethTotal gets the total amount of zeth from all bridges for a vault. The function loops over the vaultBridges array for the vault and adds the zeth value from each bridge to the zethTotal amount.

function getZethTotal(uint256 vault) internal view returns (uint256 zethTotal) {
AppStorage storage s = appStorage();
address[] storage bridges = s.vaultBridges[vault];
uint256 bridgeCount = bridges.length;
​
for (uint256 i; i < bridgeCount;) {
zethTotal += IBridge(bridges[i]).getZethValue();
unchecked {
++i;
}
}
}

If the vaultBridges array contains duplicates of the same bridge then the returned amount of getZethTotal could be incorrect if the bridge has any zeth. Since updateYield relies on the zethTotal amount, the yield would be more than it should affecting the TAPP's ethEscrowed, vault's zethYieldRate and zethCollateralRewards values.

Additionaly, the getZethTotal function is also used in the _ethConversion function in BridgeRouterFacet.sol which, if the yield was not updated, could return incorrect amounts to the withdraw, unstakeEth and withdrawTapp function leading to users getting more than they should.

When trying to delete a bridge that is duplicated, only the first occurence is removed and no other duplicates can be removed since the vault corresponding the the bridge address is deleted after removing the first duplicate. The deleteBridge function does not allow removal of a bridge with no vault linked to it.

function deleteBridge(address bridge) external onlyDAO {
uint256 vault = s.bridge[bridge].vault;
if (vault == 0) revert Errors.InvalidBridge();
​
address[] storage VaultBridges = s.vaultBridges[vault];
uint256 length = VaultBridges.length;
for (uint256 i; i < length; i++) {
if (VaultBridges[i] == bridge) {
if (i != length - 1) {
VaultBridges[i] = VaultBridges[length - 1];
}
VaultBridges.pop();
break;
}
}
delete s.bridge[bridge];
emit Events.DeleteBridge(bridge);
}

Impact

If the owner manages to push a duplicate bridge to a vault, the protocol would suffer major accounting issues which could lead to loss of funds and potentially break the functionality of the protocol since the bridge cannot be removed from the vault.

Tools Used

Manual review

Recommendation

Do not allow duplicates in the vaultBridges array for a vault by checking the bridges vault address is 0.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 2 years ago
0xnevi Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-360

Support

FAQs

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