search

code-423n4 / 2023-09-goodentry-mitigation-findings

0 stars 0 forks source link

M-08 Unmitigated #41

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L156 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L174 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L192

Vulnerability details

Comments

The success of low-level calls is not checked in V3Proxy. If msg.sender is a contract and the fallback function has additional logic, the protocol will succeed transfer by default, which will result in the loss of user funds.

Mitigation

There is no corresponding repair link posted in the readme, and there are no related repairs in https://github.com/GoodEntry-io/ge. The fix link in the issue is marked as https://github.com/GoodEntry-io/ge/pull/3/files. I think this is a misunderstanding. The sponsor mistook the issue for transfer and modified it to a call.

Suggestion

What needs to be fixed is the low-level calls in the V3Proxy contract. Their success return values ​​should be checked for success. Code positioning can be done based on the three links I mentioned above.

Conclusion

Need repair

Assessed type

call/delegatecall

c4-judge commented 12 months ago

gzeon-c4 marked the issue as unsatisfactory: Insufficient quality

gzeon-c4 commented 12 months ago

~It's mitigated in https://github.com/GoodEntry-io/ge/pull/14 which is technically linked in https://github.com/code-423n4/2023-09-goodentry-mitigation#branch but without the actual link to the PR.~

xuwinnie commented 12 months ago

🤣seems PR14 is made just after mitigation contest thanks to the finding @gzeon-c4

gzeon-c4 commented 12 months ago

🤣seems PR14 is made just after mitigation contest thanks to the finding @gzeon-c4

ah good catch thanks

c4-judge commented 12 months ago

gzeon-c4 marked the issue as satisfactory

c4-judge commented 12 months ago

gzeon-c4 marked the issue as confirmed for report

gzeon-c4 commented 11 months ago

@Keref please note that the fix in PR14 is faulty due to a duplicated transfer here

Keref commented 11 months ago

Thanks, been fixed in commit