search

code-423n4 / 2021-08-floatcapital-findings

0 stars 0 forks source link

Single Source of Truth #61

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

hickuphh3

Vulnerability details

Impact

Having spoke to the developers, it is intended for the treasury address to be the same across all contracts. As such, instead of having the address as a constructor / initializer argument in multiple contracts, it might be better to set it in 1 contract and have all other contracts pull the address from it.

That way, should the treasury be changed / upgraded, the changes will also be automatically reflected in the other contracts, thereby avoiding potential erroneous updates in multiple contracts.

Recommended Mitigation Steps

If we assume LongShort to be the source of truth, then its interface will include a treasury() function that returns the address of the treasury.

YieldManagerAave (and future yield managers) and / or Staker can then pull the address from it whenever its value is needed. An example implementation (YieldManagerAave's withdrawTreasuryFunds()) is given below.

function withdrawTreasuryFunds() external override {
  uint256 amountToWithdrawForTreasury = totalReservedForTreasury;
  totalReservedForTreasury = 0;

  // Redeem aToken for payment tokens.
  lendingPool.withdraw(address(paymentToken), amountToWithdrawForTreasury, ILongShort(longShort).treasury());

  emit WithdrawTreasuryFunds();
}
JasoonS commented 3 years ago

This is a fair point. Does have gas implications fetching values from other contracts via proxies etc etc.

Idea is that the treasury shouldn't need to change address.

JasoonS commented 3 years ago

Risk of misconfiguration extremely low, better than having the extra gas and complexity of cross contract communication.

Also can be added retro-actively to upgradable contracts.