Access control for `hook` function in `RedeemHook` Contract is inconsistent with the implementation.

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/RedeemHook.sol#L17 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L96

Vulnerability details

Impact

Access control for hook function in RedeemHook Contract is inconsistent with the implementation. Since the function involves a transfer of fees to Treasury, I've marked it as MEDIUM RISK

RedeemHook checks if sender is in a list of pre-approved accounts in _allowedMsgSenders. Access control modifier for the hook function (onlyAllowedMsgSenders) allows any allowed msg sender to access this hook.

However, the function wraps a IPrePOMarket inteface on msg.sender in line 21

         IPrePOMarket(msg.sender).getCollateral().transferFrom(msg.sender, _treasury, fee);

This ineffect means that the hook is expecting msg.sender to be PrePOMarket address. However, there is no such restriction to accounts added to _allowedMsgSenders. This behavior is inconsistent with 'access permissions' and the hook will throw an error when it tries to search for collateral address

Proof of Concept

Bob is in the list of accounts of _allowedMsgSenders and hence onlyAllowedMsgSenders allows Bob to access the redeemHook.

However, Bob's account is not a PrePOMarket - it will not return collateral address when getCollateral function is called in Line 21 & hence transferFrom function can never work when Bob calls this function.

Tools Used

Recommended Mitigation Steps

Since redeemHook will only be called from within the redeem function of PrePOMarket, appropriate access control is to ONLY allow PrePOMarket address to access the redeemHook

[Picodes]

There will be multiple PrePOMarket, and the intent is that the list of PrePOMarket is in the allowedMsgSenders list. Safety checks could be added, but this would fall within QA.

[c4-judge]

Picodes marked the issue as unsatisfactory: Overinflated severity