Compromised or malicious owner can lead into unexpected withdrawals

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L161 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L170 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L652-L658

Vulnerability details

Compromised or malicious owner can lead into unexpected withdrawals

Impact

An scenario where ownership is compromised or a malicious owner appears can lead into:

• use the unlock function to turn the maxPenalty into 0 (this function is irreversible and have no timelock)
• the owner can also set a new blocklist (for example himself) that can force undelegations
• as the penalty amount to pay can be set to 0 withdrawing will withdraw more than expected.
• also can change the penaltyRecipient to some address he owns and collectPenalty

References

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ https://blog.openzeppelin.com/opyn-gamma-protocol-audit/

Proof of Concept

unlock: https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L161 forceUndelegate: https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L170

quitLock (maxPenalty used by calling _calculatePenaltyRate) https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L652-L658

penaltyRecipient https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L153-L157

collectPenalties https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L672-L678

Recommended Mitigation Steps

• Consider a source of double check/signature for the code when unlock is called
• Consider using a timelock for this unlock scenario
• Consider not being irreversible maxPenalty value

[lacoop6tu]

The contract owner is going to be a timelock contract which is governed by the DAO through a set of smart contracts (consilium)

[gititGoro]

The comment on line 138 of VotingEscrow.sol indicates the dev understood and intended the decision. Warden reports on issues that are intentional design choices aren't helpful for the sake of auditing. Marking invalid.