`buyShareCredFor` passes the wrong curator to `_handleTrade`

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L592

Vulnerability details

Impact

When we execute buyShareCredFor, we insert a credId_, amount_, maxPrice_ and most importantly curator_ who is the person receiving the shares. However, the _handleTrade function states that the curator_ is the "address performing the action" which in this case, if X is buying a cred for Y, then clearly X is the curator not Y, yet _handleTrade in this sequence would register Y as the curator and not X. This becomes a bigger problem when you account for the fact that the function performs an if check on block.timestamp in respect of lastTradeTimestamp which invokes credId_ and curator_, but also applies the SHARE_LOCK_PERIOD check when in the else block. This ultimately means that sometimes when curator_ is interpreted in this function, it does not correlate to the accurate curator and block.timestamp checks are being made for the wrong curator, which may enable the bypass of certain checks, such as those relating directly to the curator_.

Proof of Concept

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L592

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L625-L632

Tools Used

Manual review

Recommended Mitigation Steps

Make sure when a user executes buyShareCredFor then _handleTrade accounts for msg.sender as the initial executor, but passes the receiver of shares as the correct curator later on and there is a clear distinction between msg.sender and the curator. When executing a buyShareCredFor operation, I believe msg.sender should also be affected with lastTradeTimestamp as then they essentially bypass it - and instead the receiver of shares is eligible to it - which is unfair on other users.

Assessed type

Invalid Validation