-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
destination
address can't actually call transferOut()
of UserEscrow.sol
#653
Comments
raymondfam marked the issue as sufficient quality report |
raymondfam marked the issue as duplicate of #217 |
gzeon-c4 marked the issue as not a duplicate |
gzeon-c4 marked the issue as primary issue |
gzeon-c4 marked the issue as unsatisfactory: |
gzeon-c4 removed the grade |
This is called from InvestmentManager which is a ward of UserEscrow, however it still seems to require auth on the IM to call anything to lead to transferOut. Looks like an ambiguous comment |
Yeah the implementation is correct, but the comment is wrong/misleading. It should be |
gzeon-c4 changed the severity to QA (Quality Assurance) |
Lines of code
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/UserEscrow.sol#L36-L50
Vulnerability details
Impact
The @dev comment of
transferOut
function suggeststransferOut can only be initiated by the destination address or an authorized admin
butauth
modifier prevents that. It only allows selectedwards
but making everydestination
address a ward is not feasible asward
inauth
modifier is a trusted role and has access to all the major functionality in the protocol.It bricks a major functionality. The tokens can only be transferred out by authorized ward. Thus, the whole design of
transferOut
is faulty.Proof of Concept
Not required
Tools Used
Manual Analysis
Recommended Mitigation Steps
This can be mitigated by only allowing authorized
ward
to calltransferOut
function.Assessed type
Other
The text was updated successfully, but these errors were encountered: