Platform: Code4rena
Start Date: 14/09/2022
Pot Size: $50,000 USDC
Total HM: 25
Participants: 110
Period: 5 days
Judge: hickuphh3
Total Solo HM: 9
Id: 162
League: ETH
Rank: 9/110
Findings: 1
Award: $1,541.14
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: yixxas
1541.1415 USDC - $1,541.14
https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L295 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L360-L366
Admin can rug all user funds in every vaults deployed by changing the controller address.
Controller can be changed by admin anytime without any warning to users.
function changeController(address _controller) public onlyFactory { if(_controller == address(0)) revert AddressZero(); controller = _controller; }
Tokens in the vaults can then be called by the malicious contract to be transferred to their own address with sendTokens().
function sendTokens(uint256 id, address _counterparty) public onlyController marketExists(id) { asset.transfer(_counterparty, idFinalTVL[id]); }
Allow the change of controller address only in the VaultFactory(). This way, markets that have been created cannot have a different controller address, so users can be made aware of the change before choosing to make deposit of assets.
#0 - MiguelBits
2022-09-30T00:23:43Z
implemented timelock as issued in another finding
#1 - HickupHH3
2022-11-01T14:26:50Z
admin privilege finding, rationale for QA explained here
#2 - HickupHH3
2022-11-01T14:27:35Z
user's primary QA
#3 - yixxas
2022-11-10T20:20:48Z
Hi @HickupHH3. Would like to seek clarifications on why this was downgraded to QA. In the rationale given, "Those that explained the impact and vulnerability in detail will be grouped together with medium severity ".
I believe the way a compromised admin can rug is clear in this report. changeController() can be used to change controller to any address. This address can then be used to call sendTokens() to steal all assets in every vault.
#4 - HickupHH3
2022-11-11T03:33:00Z
Took another look at this.
I disagree with the recommended fix. The purpose for having the controller changeable in the deployed instances is well intentioned for upgradeability purposes: perhaps new features are to be added to the controller, and migration to a new controller for all existing vaults is required to incur less technical debt. Needing to maintain legacy controllers isn't great from a devops POV.
That said, based on my rationale, the issue should stand as a medium severity issue until we have the introduction of centralisation reports. Kenzo said it well:
IMO most of these trusted-actor issues basically just describe general properties of the crypto/governance ecosystems, and do not reflect a novel problem in the design/implementation (which wardens are paid to discover). Because of this, and because of the circular logic, I believe we should change the rules and add a dedicated centralization report.