Admin can steal funds from vesters on AuraVestedEscrow

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L77
https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L186

Vulnerability details

Impact

Admin can steal funds from vesters on AuraVestedEscrow

Proof of Concept

By changing the address of the aura locker the admin can use the external call with funds approval to steal funds when claim() is called.
https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L77

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L186
If a malicious contract is introduced instead of aura locker the admin can steal funds from the claim since there is an external call with approval.

Recommended Mitigation Steps

Make the locker address immutable to improve the trustlessness of the protocol.

[0xMaharishi]

a) locker address may change in the future. b) claimers can choose whether to lock or not. c) admin has control to cancel the vesting stream so they already have write access

[dmvt]

I am downgrading this to QA. The report assumes that the admin is a bad actor. If the admin is a bad actor, there are many ways that they can rug the users. This is a given in any permissioned protocol. Removing admin functionality decreases the function and utility of the protocol and ultimately has a greater harm on the users. Admin functionality existing is not a medium risk issue, but a low risk one given the likelihood of the compromise occurring.

[dmvt]

Grouping this with the warden’s QA report, #282